oauth icon indicating copy to clipboard operation
oauth copied to clipboard

PollToken should cancel early if the context is canceled

Open tomlazar opened this issue 4 years ago • 3 comments

This is kind of a nice issue, but in a console app I am working on the app intercepts the os SIGKILL/SIGINT signals to do all of its own graceful shutdown via context, but currently there is no way to cancel the loop in PollToken.

To make this cancelable, it would be nice to something like this into the loop there.

		select {
		case <-ctx.Done():
			return nil, ErrCanceled
		default:
			timeSleep(checkInterval)
		}

Then the function could be extended like a lot of packages with a second copy of the method, and context propagated down.


func PollTokenContext(ctx context.Context, c httpClient, pollURL string, clientID string, code *CodeResponse) (*api.AccessToken, error) {
... current implementation
}

func PollToken(c httpClient, pollURL string, clientID string, code *CodeResponse) (*api.AccessToken, error) {
  return PollTokenContext(context.Background(), c, pollURL, clientID, code)
}

The same thing could be done to DetectFlow, or it could be embbed within the flow struct so the outer api layout would change less.

tomlazar avatar Mar 25 '21 04:03 tomlazar

That's an excellent suggestion. Thank you!

mislav avatar Mar 30 '21 09:03 mislav

I'd love to contribute a PR for this would you like the context to go all the way down from detect flow or just be embedded in that struct?

tomlazar avatar Mar 30 '21 15:03 tomlazar

@tomlazar I think that Context should be generally accepted as argument to separate methods, like you proposed, but for the users of the Flow struct, maybe context should also be an additional field on that struct? I'm not sure if adding it to a struct goes against some conventions in how contexts are used in Go. I don't have a lot of experience with them.

You are very welcome to propose a change!

mislav avatar Apr 12 '21 07:04 mislav