HTTP/3: Potential race between AddConn and RoundTrip

[danielboros]

I believe there's a race between the two paths when using HTTP/3 for the transport. I found this when hitting a panic here in internal/http3/transport.go:

return conn, t.newClientConn(conn), nil

I had a suspicion here because the wrapping path (transport.go) was recently refactored to use http3.Transport instead of http3.RoundTripper. This only happens intermittently but it is prevalent when using imroc/req in a highly trafficked environment that generates a bunch of custom clients.

Anyway, this t.newClientConn is initialized by:

func (t *Transport) init() error

... which normally is initialized inside roundTripOpt:

t.initOnce.Do(func() { t.initErr = t.init() })

Except this doesn't happen when calling this separate path:

// AddConn add a http3 connection, dial new conn if not exists.
func (t *Transport) AddConn(ctx context.Context, addr string) error {
	addr = authorityAddr(addr)
	cl, _, err := t.getClient(ctx, addr, false)
	if err == nil {
		cl.useCount.Add(-1)
	}
	return err
}

The whole chain to handlePendingAltSvc seems like it can potentially race after launching the Goroutine, I think. Let me know what you think.

[imroc]

Do you have any panic log? Is https://github.com/imroc/req/issues/456 related

[danielboros]

We're maintaining a fork but this is the path. The line numbers should be the same as v3.54.1.

goroutine 104066 [running]:
github.com/imroc/req/v3/internal/http3.(*Transport).dial(0xc011eeda40, {0x3934360, 0xc0101b8140}, {0xc0135c1ed8, 0x11})
        /go/pkg/mod/github.com/banyansecurity/req/[email protected]/internal/http3/transport.go:405 +0x21a
github.com/imroc/req/v3/internal/http3.(*Transport).getClient.func1()
        /go/pkg/mod/github.com/banyansecurity/req/[email protected]/internal/http3/transport.go:328 +0x71
created by github.com/imroc/req/v3/internal/http3.(*Transport).getClient in goroutine 104083
        /go/pkg/mod/github.com/banyansecurity/req/[email protected]/internal/http3/transport.go:325 +0x292

This change appears to have fixed it and we no longer see any intermittent panics:

https://github.com/banyansecurity/req/commit/ff7086def343a54fb501a138ed0fe0644e6caa62