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

Context is only partially supported through the Prepare->Send->Respond pipeline #314

Closed
tombuildsstuff opened this issue Sep 5, 2018 · 1 comment

Comments

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Sep 5, 2018

👋

I’ve been working through adding support for custom resource timeouts to Terraform - which means we’ve been adding Context to every field.

During this process I’ve noticed that Go-AutoRest doesn’t handle context properly - and only does so within the “Prepare” method (whereas Sender and Responder means this doesn’t work as expected). The current behaviour is to add the PollingDuration field (set at the Client level) to the passed in context and run with that.

Unfortunately we’re unable to use the PollingDuration field as it’s scoped to the client, which means since we reuse the clients (to reuse auth tokens) - setting the PollingDuration at this level means it’s not possible to reuse clients, since there’s the potential for conflicts where different callers have different timeouts (and thus Caller B’s timeout impacts Caller A).

Taking a look at the code, I believe it’s possible to solve this by making the PollingDuration field on the client nilable (read: a pointer) - which means nil-checks could be used to either use the timeout from the context cancelling (if it’s nil) - or to use the current behaviour (if a timeout is specified). This allows users of the SDK to pass in a context that’s pre-configured (with a time-out and the “cancel” method deferred) which means the context is scoped to the given API call (which in most cases should be the WaitForCompletionRef method).

Since I was prototyping this anyway whilst investigating it - I've opened #315 which includes this proposed change, but it’s as yet untested (I’ll handle that shortly) - but I figured I’d check for feedback before proceeding any further - what do you think? :)

Thanks!

@jhendrixMSFT jhendrixMSFT mentioned this issue Sep 28, 2018
4 tasks
@jhendrixMSFT
Copy link
Member

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

No branches or pull requests

2 participants