Skip to content

Commit 00cc508

Browse files
jacdevosjacdevos
committed
Prevent ExecuteTaskAsync from throwing exceptions to make async error handling more consistent with sync
1 parent 721e360 commit 00cc508

File tree

4 files changed

+58
-74
lines changed

4 files changed

+58
-74
lines changed

‎RestSharp.IntegrationTests/AsyncTests.cs‎

Lines changed: 14 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -87,30 +87,16 @@ public void Can_Handle_Exception_Thrown_By_OnBeforeDeserialization_Handler()
8787
var client = new RestClient(baseUrl);
8888
var request = new RestRequest("success");
8989

90-
request.OnBeforeDeserialization += response =>
90+
request.OnBeforeDeserialization += r =>
9191
{
9292
throw new Exception(ExceptionMessage);
9393
};
9494

9595
var task = client.ExecuteTaskAsync<Response>(request);
96-
97-
try
98-
{
99-
// In the broken version of the code, an exception thrown in OnBeforeDeserialization causes the task to
100-
// never complete. In order to test that condition, we'll wait for 5 seconds for the task to complete.
101-
// Since we're connecting to a local server, if the task hasn't completed in 5 seconds, it's safe to assume
102-
// that it will never complete.
103-
Assert.True(task.Wait(TimeSpan.FromSeconds(5)),
104-
"It looks like the async task is stuck and is never going to complete.");
105-
}
106-
catch (AggregateException e)
107-
{
108-
Assert.Equal(1, e.InnerExceptions.Count);
109-
Assert.Equal(ExceptionMessage, e.InnerExceptions.First().Message);
110-
return;
111-
}
112-
113-
Assert.True(false, "The exception thrown from OnBeforeDeserialization should have bubbled up.");
96+
task.Wait();
97+
var response = task.Result;
98+
Assert.Equal(ExceptionMessage, response.ErrorMessage);
99+
Assert.Equal(ResponseStatus.Error, response.ResponseStatus);
114100
}
115101
}
116102

@@ -233,15 +219,12 @@ public void Can_Timeout_GET_TaskAsync()
233219
//Half the value of ResponseHandler.Timeout
234220
request.Timeout = 500;
235221

236-
AggregateException agg = Assert.Throws<AggregateException>(
237-
delegate
238-
{
239-
var task = client.ExecuteTaskAsync(request);
240-
task.Wait();
241-
});
242222

243-
Assert.IsType(typeof(WebException), agg.InnerException);
244-
Assert.Equal("The request timed-out.", agg.InnerException.Message);
223+
var task = client.ExecuteTaskAsync(request);
224+
task.Wait();
225+
var response = task.Result;
226+
Assert.Equal(ResponseStatus.TimedOut, response.ResponseStatus);
227+
245228
}
246229
}
247230

@@ -258,15 +241,10 @@ public void Can_Timeout_PUT_TaskAsync()
258241
//Half the value of ResponseHandler.Timeout
259242
request.Timeout = 500;
260243

261-
AggregateException agg = Assert.Throws<AggregateException>(
262-
delegate
263-
{
264-
var task = client.ExecuteTaskAsync(request);
265-
task.Wait();
266-
});
267-
268-
Assert.IsType(typeof(WebException), agg.InnerException);
269-
Assert.Equal("The request timed-out.", agg.InnerException.Message);
244+
var task = client.ExecuteTaskAsync(request);
245+
task.Wait();
246+
var response = task.Result;
247+
Assert.Equal(ResponseStatus.TimedOut, response.ResponseStatus);
270248
}
271249
}
272250

‎RestSharp.IntegrationTests/NonProtocolExceptionHandlingTests.cs‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,22 +37,14 @@ public void Task_Handles_Non_Existent_Domain()
3737
Method = Method.GET
3838
};
3939

40-
AggregateException agg = Assert.Throws<AggregateException>(
41-
delegate
42-
{
43-
var response = client.ExecuteTaskAsync<StupidClass>(request);
40+
var task = client.ExecuteTaskAsync<StupidClass>(request);
41+
task.Wait();
4442

45-
response.Wait();
46-
});
43+
var response = task.Result;
4744

48-
Assert.IsType(typeof(WebException), agg.InnerException);
49-
Assert.Equal("Unable to connect to the remote server", agg.InnerException.Message);
50-
51-
//var client = new RestClient("http://nonexistantdomainimguessing.org");
52-
//var request = new RestRequest("foo");
53-
//var response = client.ExecuteTaskAsync(request);
54-
55-
//Assert.Equal(ResponseStatus.Error, response.Result.ResponseStatus);
45+
Assert.IsType(typeof(WebException), response.ErrorException);
46+
Assert.Equal("Unable to connect to the remote server", response.ErrorException.Message);
47+
Assert.Equal(ResponseStatus.Error, response.ResponseStatus);
5648
}
5749

5850
/// <summary>
@@ -73,14 +65,11 @@ public void Handles_Server_Timeout_Error()
7365

7466
Assert.NotNull(response.ErrorException);
7567
Assert.IsAssignableFrom(typeof(WebException), response.ErrorException);
76-
Assert.Equal("The operation has timed out", response.ErrorException.Message);
68+
Assert.Contains("The operation has timed out", response.ErrorException.Message);
7769
}
7870
}
7971

8072
[Fact]
81-
// The asserts get trapped by a catch block and then added to the response.
82-
// Then the second assert is hit and it just hangs indefinitely.
83-
// Not sure why it can't break out.
8473
public void Handles_Server_Timeout_Error_Async()
8574
{
8675
const string baseUrl = "http://localhost:8888/";
@@ -102,10 +91,31 @@ public void Handles_Server_Timeout_Error_Async()
10291

10392
Assert.NotNull(response);
10493
Assert.Equal(response.ResponseStatus, ResponseStatus.TimedOut);
94+
Assert.NotNull(response.ErrorException);
95+
Assert.IsAssignableFrom(typeof(WebException), response.ErrorException);
96+
Assert.Equal(response.ErrorException.Message, "The request timed-out.");
97+
}
98+
}
99+
100+
[Fact]
101+
public void Handles_Server_Timeout_Error_AsyncTask()
102+
{
103+
const string baseUrl = "http://localhost:8888/";
105104

106-
//Assert.NotNull(response.ErrorException);
107-
//Assert.IsAssignableFrom(typeof(WebException), response.ErrorException);
108-
//Assert.Equal(response.ErrorException.Message, "The operation has timed out");
105+
using (SimpleServer.Create(baseUrl, TimeoutHandler))
106+
{
107+
var client = new RestClient(baseUrl);
108+
var request = new RestRequest("404") { Timeout = 500 };
109+
110+
var task = client.ExecuteTaskAsync(request);
111+
task.Wait();
112+
IRestResponse response = task.Result;
113+
Assert.NotNull(response);
114+
Assert.Equal(response.ResponseStatus, ResponseStatus.TimedOut);
115+
116+
Assert.NotNull(response.ErrorException);
117+
Assert.IsAssignableFrom(typeof(WebException), response.ErrorException);
118+
Assert.Equal(response.ErrorException.Message, "The request timed-out.");
109119
}
110120
}
111121

@@ -128,7 +138,7 @@ public void Handles_Server_Timeout_Error_With_Deserializer()
128138
Assert.Null(response.Data);
129139
Assert.NotNull(response.ErrorException);
130140
Assert.IsAssignableFrom(typeof(WebException), response.ErrorException);
131-
Assert.Equal("The operation has timed out", response.ErrorException.Message);
141+
Assert.Contains("The operation has timed out", response.ErrorException.Message);
132142
}
133143
}
134144

‎RestSharp/Http.Async.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,9 +368,19 @@ private void ResponseCallback(IAsyncResult result, Action<HttpResponse> callback
368368

369369
private static void ExecuteCallback(HttpResponse response, Action<HttpResponse> callback)
370370
{
371+
PopulateErrorForIncompleteResponse(response);
371372
callback(response);
372373
}
373374

375+
private static void PopulateErrorForIncompleteResponse(HttpResponse response)
376+
{
377+
if (response.ResponseStatus != ResponseStatus.Completed && response.ErrorException == null)
378+
{
379+
response.ErrorException = response.ResponseStatus.ToWebException();
380+
response.ErrorMessage = response.ErrorException.Message;
381+
}
382+
}
383+
374384
partial void AddAsyncHeaderActions()
375385
{
376386
#if SILVERLIGHT

‎RestSharp/RestClient.Async.cs‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -275,14 +275,7 @@ public virtual Task<IRestResponse<T>> ExecuteTaskAsync<T>(IRestRequest request,
275275
{
276276
taskCompletionSource.TrySetCanceled();
277277
}
278-
else if (response.ErrorException != null)
279-
{
280-
taskCompletionSource.TrySetException(response.ErrorException);
281-
}
282-
else if (response.ResponseStatus != ResponseStatus.Completed)
283-
{
284-
taskCompletionSource.TrySetException(response.ResponseStatus.ToWebException());
285-
}
278+
//Don't run TrySetException, since we should set Error properties and swallow exceptions to be consistent with sync methods
286279
else
287280
{
288281
taskCompletionSource.TrySetResult(response);
@@ -384,14 +377,7 @@ public virtual Task<IRestResponse> ExecuteTaskAsync(IRestRequest request, Cancel
384377
{
385378
taskCompletionSource.TrySetCanceled();
386379
}
387-
else if (response.ErrorException != null)
388-
{
389-
taskCompletionSource.TrySetException(response.ErrorException);
390-
}
391-
else if (response.ResponseStatus != ResponseStatus.Completed)
392-
{
393-
taskCompletionSource.TrySetException(response.ResponseStatus.ToWebException());
394-
}
380+
//Don't run TrySetException, since we should set Error properties and swallow exceptions to be consistent with sync methods
395381
else
396382
{
397383
taskCompletionSource.TrySetResult(response);

0 commit comments

Comments
 (0)