cli / oauth

A library for performing OAuth Device flow and Web application flow in Go client apps.
https://pkg.go.dev/github.com/cli/oauth
MIT License
460 stars 63 forks source link

PollToken should cancel early if the context is canceled #6

Closed tomlazar closed 1 year ago

tomlazar commented 3 years ago

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.

mislav commented 3 years ago

That's an excellent suggestion. Thank you!

tomlazar commented 3 years ago

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?

mislav commented 3 years ago

@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!