Missing TraceInfo for requests with tracing, save response, and download callback enabled

[cheeseprocedure]

Summary

When a request has tracing enabled, is configured to save output (e.g. by calling SetOutput()), and configured with a download callback, HTTP trace hooks in the request context appear to be lost, resulting in mostly zero values for the request's TraceInfo fields.

Details

In client.go v3.19.1 (the version I recently updated from), the request context is initialized and passed to the HTTP request like so, and I appear to get valid TraceInfo values when the request completes:

if r.trace == nil && r.client.trace {
	r.trace = &clientTrace{}
}
if r.trace != nil {
	r.ctx = r.trace.createContext(r.Context())
}

// ...
ctx := r.ctx

if r.isSaveResponse && r.downloadCallback != nil {
	// ...
	if ctx == nil {
		ctx = context.Background()
	}
	ctx = context.WithValue(ctx, wrapResponseBodyKey, wrap)
}
if ctx != nil {
	req = req.WithContext(ctx)
}

...but in v3.26.6, ctx is conditionally overwritten using the *req.Request's original context instead of the existing ctx, which at this point may have trace hooks registered:

if r.trace == nil && r.client.trace {
	r.trace = &clientTrace{}
}
ctx := r.ctx
if r.trace != nil {
	ctx = r.trace.createContext(r.Context())
}

// ...

if r.isSaveResponse && r.downloadCallback != nil {
	// ...
	ctx = context.WithValue(r.Context(), wrapResponseBodyKey, wrap)
}

if ctx != nil {
	req = req.WithContext(ctx)
}

With this version of the package, I see mostly zero values in the request's TraceInfo, and logging the output of httptrace.ContextClientTrace(ctx) reports (*httptrace.ClientTrace)(nil), confirming trace hooks are not present.

It appears this change was introduced while addressing an issue identified by Go's data race detector (#159).

Potential resolution

I've verified that changing the conditional line to ctx = context.WithValue(ctx, wrapResponseBodyKey, wrap) resolves the issue locally, and I cannot reproduce the data race from #159 after this patch, but I'm not certain it's a complete fix.

Thank you for your work on this project!

Environment

Go version: go version go1.19.3 darwin/amd64

[imroc]

Thanks for the feedback, this is a very hidden bug, ctx might be nil at that line, so use r.Context() to get ctx, but unfortunately, ctx might be overwritten when trace is enabled and download callback is set.

This is fixed in this commit, you can upgrade to the latest version.

[cheeseprocedure]

Thanks so much! I've confirmed the issue is resolved for us in v3.26.7.