Yep, that's technically a bug, but it seems basically unfixable given the current context and httptrace APIs. Do you have any suggestions? I'm all ears.

When we made httptrace, I guess we had implicitly assumed that each trace object was single-use, which made it "safe" to access the trace object outside of the scope of the context. I say "safe" because I don't think we actually through this problem completely.

Aside: #20617 might be related to #19643 but is unrelated to this bug.

Do you have any suggestions?

Per above, unpack the ClientTrace early and save it, and wait for DialContext to return before allowing getConn to return. The change may be ugly, but it seems straightforward.

wait for DialContext to return before allowing getConn to return. The change may be ugly, but it seems straightforward.

We can't do that, it will be a performance regression. But you're right, one way to "fix" the problem is to move all background tasks onto the critical path. I just don't think that's an acceptable fix.

We can't do that, it will be a performance regression.

If DialContext is responsive to context cancellation, then it will introduce a negligible delay if an idle connection is found. If DialContext is not responsive to cancellation, then it will fix a potential OOM condition if too many dials are in flight.

Beyond that, as far as I can tell, it will add one allocation (for the child context), and then only if the DialContext function is non-nil. I would be shocked if that has a noticeable impact relative to setting up an HTTP connection.

Sorry, I misunderstood your suggestion. You're suggesting that we cancel the pending dial if an idle conn becomes available before the dial finishes (which we currently do not do ... this is the part I missed). If the pending dial is canceled, then I agree, waiting for a canceled dial to finish does not seem like a problem.

That is still a semantics change that could affect performance. Currently, we let the pending dial finish and add it to the pool (up to Transport.MaxIdleConns and Transport.MaxIdleConnsPerHost). If we cancel the pending dial, that means a follow-up request will need to wait for a new dial or for a prior request to finish. This is potentially slower than the current implementation, where the follow-up request might use the pending dial. I am sure some usage pattern of http.Transport would be harmed by this change, and eventually we'd get a bug report.

I consider it a bug that we dial using the request's ctx. Ideally we'd dial using a background ctx, but that depends on #19643, which is unlikely to be fixed any time soon.

I think I have a good (Google-internal) project that could act as a benchmark for your suggestion. If there's no performance impact for that project, then your suggestion is likely a good solution.

Moving to Go 1.11.