Added support for cancellation tokens #178
Conversation
…gov#130 - await rather than accessing task result.
| @@ -34,55 +35,71 @@ public BaseClient(IHttpClient client, string apiKey, string baseUrl = "https://a | |||
| this.client.BaseAddress = ValidateBaseUri(BaseUrl); | |||
| this.client.AddContentHeader("application/json"); | |||
|
|
|||
| var productVersion = typeof(BaseClient).GetTypeInfo().Assembly.GetName().Version.ToString(); | |||
| var productVersion = typeof(BaseClient).GetTypeInfo().Assembly.GetName().Version?.ToString(); | |||
There was a problem hiding this comment.
This change is required as there was the possibility of a NullReferenceException in the original code - the Version attribute may be null.
If nullable types were enabled for the project it would likely help catch other similar problems.
| #if NET6_0_OR_GREATER | ||
| var responseContent = await response.Content.ReadAsByteArrayAsync(cancellationToken) | ||
| .ConfigureAwait(false); | ||
| #else | ||
| var responseContent = await response.Content.ReadAsByteArrayAsync() | ||
| .ConfigureAwait(false); | ||
| #endif |
There was a problem hiding this comment.
Passing cancellation tokens when reading content is only supported in .NET standard 2.1 and up. This project currently targets .net standard 2.0 and .net 6, so this conditional compilation should work.
| #if NET6_0_OR_GREATER | ||
| var responseContent = await response.Content.ReadAsStringAsync(cancellationToken) | ||
| .ConfigureAwait(false); | ||
| #else | ||
| var responseContent = await response.Content.ReadAsStringAsync() | ||
| .ConfigureAwait(false); | ||
| #endif |
There was a problem hiding this comment.
Again, as before this conditional compilation is required due to the target framework.
| } | ||
| catch (AggregateException ae) | ||
| { | ||
| ae.Handle(x => | ||
| { | ||
| if (x is HttpRequestException) | ||
| { | ||
| throw new NotifyClientException(x.InnerException.Message); | ||
| throw new NotifyClientException(x.InnerException?.Message ?? x.Message); |
There was a problem hiding this comment.
It's possible for InnerException to be null here, which would lead to an uncaught NullReferenceException.
| if (string.IsNullOrWhiteSpace(fromApiKey) || fromApiKey.Length < 74 || fromApiKey.Contains(' ')) | ||
| #else | ||
| if (string.IsNullOrWhiteSpace(fromApiKey) || fromApiKey.Length < 74 || fromApiKey.Contains(" ")) |
There was a problem hiding this comment.
In the original code the .Length property was accessed before checking for null, which led to NullReferenceExceptions when an api key was not set.
| public void Dispose() | ||
| { | ||
| _client.Dispose(); | ||
| Dispose(true); | ||
| GC.SuppressFinalize(this); | ||
| } | ||
|
|
||
| protected virtual void Dispose(bool disposing) | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (disposing) | ||
| { | ||
| _client.Dispose(); | ||
| } | ||
|
|
||
| _disposed = true; | ||
| } |
There was a problem hiding this comment.
This change is to implement the dispose pattern as recommended by Microsoft:
https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern
| } | ||
|
|
||
| if (ex.InnerExceptions != null && ex.InnerExceptions.Count == 1) | ||
| if (ex.InnerExceptions.Count == 1) |
There was a problem hiding this comment.
InnerExceptions cannot be null, so there's no need for a null check here.
|
I'm not sure why but it looks like the CI may be stalled on this one. Would someone mind giving it a nudge? The changes mentioned in the previous comment will be explored in a future PR after this one has been merged. |
What problem does the pull request solve?
Fixes #176 by adding support for passing
CancellationTokenparameters to asynchronous methods.Also fixes #130 - rather than accessing a
Taskresult directly the task should beawaited.These changes allow early cancellation of requests.
Checklist
DOCUMENATION.mdandCHANGELOG.md)src/GovukNotify/GovukNotify.csproj)