Fix: Provide empty context without deadline

[gogolok]

The request context has set up a deadline of value DefaultRequestTimeout. The function CreateOAuth2TokenSource would store that context in the token source creator. The token source creator would later try to create a new Token(), but fail since the deadline would be reached.

The context passed to oauth2 package controls which HTTP client is used. See the oauth2.HTTPClient variable.

Therefore fix the situation by passing an empty context to oauth2 package.

Fixes #444

[gogolok]

https://github.com/golang/oauth2/blob/314ee5b92bf23c4973aa8e61eba3ff458e80eef2/internal/transport.go#L23
https://github.com/golang/oauth2/blob/314ee5b92bf23c4973aa8e61eba3ff458e80eef2/oauth2.go#L197
https://github.com/golang/oauth2/blob/314ee5b92bf23c4973aa8e61eba3ff458e80eef2/oauth2.go#L341

[sneal]

Thanks for digging into this. Holding onto the context like that definitely seems like the problem and this PR seems like it would address it. I think the only I worry I have is this may make any requests to refresh the oauth token non-cancellable since it uses a new background context. While that may be a problem in some scenarios, it seems like less of a problem than the one you encountered for long running programs. The real problem seems to be the caching of the request context for the duration of the process. It's been a while since I've really dug into this oauth code, so it's very possible I'm misunderstanding something.

Out of curiosity, do you know if providing a new context for every call through the loop like stacks, _, err := cf.Stacks.List(context.Background(), nil) has the same problem?

[gogolok]

I must admit it's really challenging (to me) to go through go-cfclient, oauth2 and net/http code base.
.oO(In the end I've added even more 'debug' output and even draw some things on a paper after your last comment :-) )

But it's also challenging to understand how others use go-cfclient and contexts in general and what are the expectations.

1. When you use the go-cfclient client to fetch the stacks list for example, you can pass a context to the list operation. That context becomes a request with a request context and competes with the Config.requestTimeout value. Whatever deadline comes first will win. So in your question regarding "Out of curiosity, do you know if providing a new context for every call through the loop":
   • The Config.requestTimeout (by default 30s) would win and the request would definitely be canceled if it takes too long.
     So even though you set a context.Background() without a deadline, there would internally be a deadline via Config.requestTimeout.
   • Therefore, the same issue would happen since CreateOAuth2TokenSource would get the request's deadline in any case.
2. "I think the only I worry I have is this may make any requests to refresh the oauth token non-cancellable since it uses a new background context."
   Could you elaborate on that? Do you want to be able to globally cancel the token source (creator) at any time from outside? Or do you fear that I've lost the old http client?

I guess a different and probably better solution than the current one in the commit/'PR change' is:

The token refresher that is being created with

func (c *Config) CreateOAuth2TokenSource(ctx context.Context) {
   	// use our http.Client instance for token acquisition
    oauthCtx := context.WithValue(ctx, oauth2.HTTPClient, c.httpClient)

copies all deadlines from the parent (the current request context).
The deadlines are not relative (not timeout values), but absolute time values. Here some debug output:

oldCtx = context.Background.WithDeadline(2025-02-07 17:04:40.152509 +0100 CET m=+270.350795917 [-3m30.450063459s]).WithValue(internal.ContextKey, *http.Client)

If we just need a context with the old http client, we could probably copy it over to a new context.Background() context and not copy over the previous deadlines.
This way we would get a http request with a timeout and the CreateOAuth2TokenSource token source creator could/would still get a request timeout (by still having the http client).

diff --git a/vendor/github.com/cloudfoundry/go-cfclient/v3/config/config.go b/vendor/github.com/cloudfoundry/go-cfclient/v3/config/config.go
index dd1c992..71359eb 100644
--- a/vendor/github.com/cloudfoundry/go-cfclient/v3/config/config.go
+++ b/vendor/github.com/cloudfoundry/go-cfclient/v3/config/config.go
@@ -123,7 +123,8 @@ func (c *Config) AuthURL(urlPath string) string {
 // on-demand.
 func (c *Config) CreateOAuth2TokenSource(ctx context.Context) (oauth2.TokenSource, error) {
        // use our http.Client instance for token acquisition
-       oauthCtx := context.WithValue(ctx, oauth2.HTTPClient, c.httpClient)
+       oauthCtx := context.WithValue(context.Background(), oauth2.HTTPClient, c.httpClient)

[sneal]

Most go-cfclient consumers probably just use the request timeout in the config. While probably rarely used with this client, using a cancellable context when making a request allows you cancel inflight requests running in a goroutine:

ctx, cancel := context.WithCancel(context.Background())
cf, _ := client.New(cfg)
go func() {
	orgs, _ := cf.Organizations.ListAll(ctx, nil)
	for _, o := range orgs {
		fmt.Printf("%s\n", o.Name)
	}
}()
time.Sleep(100 * time.Millisecond)
cancel()

My main worry was that using a new context for the oauth call might make that uncancellable.

[gogolok]

Thanks for the sample code. I will try to check on Monday whether I can give a better answer with WithCancel in mind.

[gogolok]

I've created a second pull request #447 and the following text is referring to that PR and my latest state :-)

• The token source will always get a fresh context with background http.Client. I think this is essential since the token source is independant of the current request at hand and will be used in future requests. If you would store a context with a deadline (from the current request) in oauthCtx, it gets unusable for anything after the deadline hits us and we're stuck.
• In the case of a password grant type: The call to PasswordCredentialsToken would use the request context (with the deadline of 10 milliseconds for example). This ensures the cancel works for password grant type and doesn't run 'forever' for example.

The token source does not get a request context during the Token() call: https://github.com/golang/oauth2/blob/314ee5b92bf23c4973aa8e61eba3ff458e80eef2/transport.go#L45
People mentioned this in different GitHub issues already.

Regarding your example scenario in
#445 (comment)
You will always get a 'cancel' event.

First the following code gets called:

	// Retry logic
	if shouldRetryAuth(resp) {
		// We're going to retry, consume any response to reuse the connection.
		drainBody(resp)

		// Recreate the token source
		src, tsErr := t.tokenSourceCreator.CreateOAuth2TokenSource(req.Context())

This might already break in the case of password grant type within CreateOAuth2TokenSource. And if not there (since the deadline might not have expired or cancel was not called), then a new round trip with the request would be retried

		// Retry the request
		resp, err = t.transport.RoundTrip(req)

and cancel that event finally.

So I don't see a reason how the current request could not be cancelled.

I've tested so far with the base repo with https://github.com/gogolok/go-cfclient-issue-rg0/ and local modifications (to vendor/ + even the net/http code base). So if you need something else confirmed, please let me know.