Long running operations fails due to context timeout after 15 minutes even if context has no timeout

[seyadava]

This line overrides context with context.WithTimeout if PollingDuration is not set to -1.

go-autorest/autorest/azure/async.go

Line 195 in 16ac8fb

if d := client.PollingDuration; d != 0 {

PollingDuration has default value of 900 seconds (~15 minutes) in go-sdk. So if you pass a context without timeout value (such as context.Background()) your requests times out after 15 minutes with "context deadline exceeded" error. We should change the logic and honor user's context instead of setting a default value here.

[jhendrixMSFT]

My only concern is is that misbehaved LROs (or bugs in the LRO state machine) will essentially hang the process. I agree the behavior isn't very intuitive though. One idea is to omit creating our internal context with deadline if the specified context already has one, something like this.

// if the provided context already has a deadline don't override it
_, hasDeadline := ctx.Deadline()
if d := client.PollingDuration; !hasDeadline && d != 0 {
	var cancel context.CancelFunc
	cancelCtx, cancel = context.WithTimeout(ctx, d)
	defer cancel()
}

Of course this doesn't address the case where you really want it to wait forever, and setting the PollingDuration on the client to zero is a bit clunky especially if you want it to apply to some API calls and not others. I suppose you could just create a context with a really long deadline but that seems a bit clunky too.
CC @tombuildsstuff

[seyadava]

Your suggested change can solve our issue. I can pass a context with a long deadline. But like you said, that is not intuitive and we may end up with some users who want to use Background context (w/o deadline) instead of context with long deadline. However, it seems we need to find a trade-off between misbehaved LROs and no deadline context.
If we move forward with your suggested change we should document that properly and let users know that they cannot use Background context and their only way to eliminate context timeout is to set pollingDuration (in Go SDK) to zero.

CC @knithinc and @bganapa

[tombuildsstuff]

@jhendrixMSFT that looks good from our side - we’ve had to put the context work on hold for the moment but wanted to pick it up again in the next couple of weeks anyway, so good timing heh

At the moment we default all timeouts to an hour - so we were planning to do the same for contexts (for the most part) and then let users override it where necessary; so I believe that’ll work for us 👍

[jhendrixMSFT]

Fixed in https://github.com/Azure/go-autorest/releases/tag/v11.3.2