Create ApiRequestException for wrapping SendAsync exceptions #2052
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Feature with breaking changes
What is the current behavior?
Refit currently doesn't catch/wrap any exceptions that occur before receiving a response from the target server (e.g. host not available, DNS errors, etc). These are usually an
HttpRequestException(orTaskCanceledExceptionin the case of timeouts) thrown byHttpClient.SendAsync.When these exceptions are thrown, they're missing a lot of the detail about the request that Refit usually includes in its
ApiException. If they are caught by a global exception handler (or some other catch block far away from the context of the call), then you lose out on the ability to include that context in your error logs or add generic handling based on the request info. See #273 and #719 for old issues on this topic.These exceptions also aren't captured by
ApiResponse.Error, requiring those who useApiResponseto both have if statements for handling successful/failed responses, and try catch blocks if they wish to handle the above error cases too. See #1238 for an open issue on this topic.What is the new behavior?
Part 1
Create a new
ApiRequestExceptionwith all of the same request-related properties asApiException(all the common properties are placed in a newApiExceptionBaseclass that they both derive from). A newtry/catcharoundHttpClient.SendAsynccatches any exceptions, wraps them in the newApiRequestExceptionwith all the additional context, and rethrows it. This is a breaking change, since existing code that catches the original exceptions won't work anymore.Example usage:
Part 2
For users who use the
ApiResponsewrapper, the new exception won't be thrown, but instead placed in theApiResponse.Errorproperty and return early. This kicks off some more breaking changes:ApiResponseis no longer guaranteed to contain a response from the server. All response-related properties are now nullable (e.g. status code, content, headers).IsSuccessful/IsSuccessStatusCodehave compiler hints to mark them as not-null if true, and a newIsReceivedproperty has been added to mark them as non-null but not distinguish between successful/unsuccessful responses from the serverErrorproperty's type has been changed fromApiExceptiontoApiExceptionBase, so exception properties relating to responses aren't immediately accessible anymore. Two new methods have been added to cast to the actual exception type -bool HasRequestError(out ApiRequestException ex)(slightly redundant since all the properties are already inApiExceptionBase) andbool HasResponseError(out ApiException ex)(where you can access all the previous exception properties).Example usage:
What might this PR break?
HttpRequestExceptionand other exceptions thrown byHttpClient.SendAsync.IApiResponseby making them nullable, and also changes the base type of theErrorproperty fromApiExceptiontoApiBaseException(which leaves only the request-related properties immediately accessible)If you want to keep the number of breaking changes to a minimum, you could:
A) Scrap Part 2 of the solution. I can't think of a way of adding request exceptions to
ApiResponsewithout breaking all of the response-related propertiesB) Keep Part 1, and add a new option into
RefitSettingsso that users can choose whether they'd like their exceptions wrapped or keep the legacy functionality. Could even have it default to false initially, and switch the default in a later major version.Please check if the PR fulfills these requirements
Other information: