Request context question

[stevehipwell]

The current implementation of the client has the NewRequest method which uses http.NewRequest and then we use withContext which just wraps req.WithContext to copy the context. Is there a compelling reason why this pattern is used instead of using http.NewRequestWithContext which would save having to copy every request just to set the context which is known when the request is created?

[gmlewis]

Is there a compelling reason why this pattern is used instead of using http.NewRequestWithContext which would save having to copy every request just to set the context which is known when the request is created?

To answer that question, I will need to review the history of that code and see what issues were raised that caused it to be created in the first place.

However, before I even do that, I see that the file without_appengine.go uses build tags and has the following:

//go:build !appengine
//...

func withContext(ctx context.Context, req *http.Request) *http.Request {
	return req.WithContext(ctx)
}

and with_appengine.go also uses build tags and has the following:

//go:build appengine
//...

func withContext(ctx context.Context, req *http.Request) *http.Request {
	// No-op because App Engine adds context to a request differently.
	return req
}

So I can definitively state that yes, there is a compelling reason why this pattern is used.
One version supports running this client library on Google App Engine, and the other version outside of App Engine.

Also, I'm pretty sure that this code was written before http.NewRequestWithContext was added to the Go standard library.

Would you like to propose an alternative implementation that doesn't break current functionality?
If so, we can review your new implementation in a PR.

[stevehipwell]

@gmlewis I'm was guessing that it was added before with context was a thing. But given the history of breaking changes to this library I can't see why the default code path hasn't been optimised to use the context aware functions instead of copying each request to patch in the context.

RE app engine, I'm not familiar with the special logic required but if always using a background context is required that would be possible while still optimising the other paths.

[gmlewis]

@gmlewis I'm was guessing that it was added before with context was a thing. But given the history of breaking changes to this library I can't see why the default code path hasn't been optimised to use the context aware functions instead of copying each request to patch in the context.

Probably because nobody looked at this code again, including myself (who wrote it).

RE app engine, I'm not familiar with the special logic required but if always using a background context is required that would be possible while still optimising the other paths.

I'm totally fine with that. Thanks, @stevehipwell.