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

Wrong documentation on NewClient #564

Open
Itshardtopickanusername opened this issue May 11, 2022 · 3 comments
Open

Wrong documentation on NewClient #564

Itshardtopickanusername opened this issue May 11, 2022 · 3 comments

Comments

@Itshardtopickanusername

Documentation of NewClient states:

// The returned client is not valid beyond the lifetime of the context.

However, even if you pass a canceled context into NewClient, it has absolutely no effect.
For example:

func (c *Config) NewCustomClient(ctx context.Context) *http.Client {

    ctx2, cancel := context.WithCancel(ctx)
    cancel()
    return oauth2.NewClient(ctx2, c.TokenSource(ctx))
}

and then make request with returned client:

c := cfg.NewCustomClient(context.Background())
req, _ := http.NewRequest(http.MethodGet, getURL().String(), http.NoBody)
req.Header.Set("Accept", "application/json")
resp, err := c.Do(req)

it has no effect.

or am I misunderstanding something?

@mitar
Copy link

mitar commented Jun 7, 2022

I agree. This part of documentation seems that it should be moved to Config.Client. In there a tokenRefresher TokenSource is created, and that one stores and uses ctx and would fail if ctx is canceled.

@dnesting
Copy link

dnesting commented May 7, 2023

This may be clarified by https://go-review.googlesource.com/c/oauth2/+/493335.

There are three places contexts can be used here:

httpClient := oauth2.NewClient(newClientContext, c.TokenSource(tokenSourceContext))
req = req.WithContext(reqContext)
httpClient.Do(req)
  • newClientContext is used to configure the Transport used by httpClient. The context is not used after this, and so canceling it has no effect on the request.
  • tokenSourceContext is used to set the HTTP client used to refresh tokens. If it is canceled, token refreshes will fail. It does not influence the httpClient otherwise.
  • reqContext is used to time out/cancel a request. Its influence is limited to the request itself.

@willmo
Copy link

willmo commented Jul 12, 2023

@dnesting maybe this is a separate bug, but it seems to me that the contexts aren't actually respected the way they should be. In particular, I think that cancelling reqContext should cancel a token refresh being performed synchronously as part of handling the request, but oauth2.Transport.RoundTrip doesn't/can't pass it to t.Source.Token(). And e.g. jwtSource.Token calls hc.PostForm which doesn't accept a context, so it honors neither tokenSourceContext nor reqContext.

858c2ad correctly declared that cancellation should be done using the request context, but didn't actually make it work...

edit: Sorry, I see this was already discussed on #262.

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

4 participants