Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Cancellation Token support across all public service APIs. (Resolves #238) #476

Merged

Conversation

pianomanjh
Copy link

  • ShopifyService ExecuteXXX calls leverage a cancellation token for the underlying HttpClient request and the execution policy timeout
  • All existing Service public API methods now take an optional CancellationToken

I included some tests on the ShopifyService just to ensure the token is leveraged properly. I did not add tests to every single service API call, as they proxy requests to the base class, and token use there is tested. Therefore, please review the public service calls to ensure I didn't miss anything.

@pianomanjh pianomanjh changed the title Introduce Cancellation Token support across all public service APIs Introduce Cancellation Token support across all public service APIs. Resolves #238 Mar 9, 2020
@pianomanjh pianomanjh changed the title Introduce Cancellation Token support across all public service APIs. Resolves #238 Introduce Cancellation Token support across all public service APIs. (Resolves #238) Mar 9, 2020
Josh Huber added 2 commits March 9, 2020 19:36
…e ShopifyService Execute methods. This has the benefit of clarifying to new services the expectation that a CancellationToken should be passed and on the caller API surface.
@clement911
Copy link
Collaborator

Nice!

@nozzlegear nozzlegear merged commit ad93fdb into nozzlegear:master Apr 14, 2020
@nozzlegear
Copy link
Owner

Awesome, thank you for all the work on this! I know it was a lot of tedious changes. Sorry it took so long to merge in, I'll let the tests run and get it published soon.

@clement911
Copy link
Collaborator

thank you @pianomanjh for your contribution!

@nozzlegear
Copy link
Owner

nozzlegear commented Apr 15, 2020

Just a heads up, this seems to be randomly throwing both OperationCanceledException and TaskCanceledException, it's not consistent at the moment. Not sure if that's an implementation issue, or if it's just the nature of cancelling tasks.

@pianomanjh
Copy link
Author

pianomanjh commented Apr 15, 2020

Yeah, that is likely a difference between the exceptions thrown by consumers of the token. We pass it into both a Task.Delay and HttpClient. Both use internal timers on likely differing polling intervals to check the token. Could vary by whichever one 'notices' the cancellation first. I'll take a better look at it tonight and report back.

@nozzlegear
Copy link
Owner

Published in 5.1.0 on nuget. Thanks for the PR and all the work you put into this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants