Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http/httptrace: internal nettrace leaks into other net.Dial calls #25198

Open
DanielMorsing opened this Issue May 1, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@DanielMorsing
Copy link
Contributor

DanielMorsing commented May 1, 2018

What version of Go are you using (go version)?

go version go1.10.2 linux/amd64

Does this issue reproduce with the latest release?

Yep

What did you do?

Run the following program

package main

import (
	"context"
	"fmt"
	"net"
	"net/http"
	"net/http/httptrace"
)

func main() {
	dialFunc := func(ctx context.Context, network string, addr string) (net.Conn, error) {
		// This would be where there's a net.Dial to an external lookup service
		// but just pretend that this actually matters for the purposes of this bug
		// report
		n, err := (&net.Dialer{}).DialContext(ctx, "tcp", "www.golang.org:80")
		if err != nil {
			panic(err)
		}
		n.Close()
		// assume that 1.1.1.1 is an IP returned from the lookup service
		// This IP was purely chosen because it's an IP that I know runs a
		// HTTP server
		return (&net.Dialer{}).DialContext(ctx, network, "1.1.1.1:80")
	}

	transport := &http.Transport{
		DialContext: dialFunc,
	}
	req, err := http.NewRequest("GET", "http://example.org/", nil)
	if err != nil {
		panic(err)
	}
	ct := &httptrace.ClientTrace{
		DNSStart: func(info httptrace.DNSStartInfo) {
			fmt.Println("DNSStart:", info.Host)
		},
	}
	ctx := httptrace.WithClientTrace(context.Background(), ct)
	req = req.WithContext(ctx)
	resp, err := transport.RoundTrip(req)
	if err != nil {
		panic(err)
	}
	resp.Body.Close()
}

What did you expect to see?

Nothing printed on stdout

What did you see instead?

DNSStart: www.golang.org printed on stdout

Additionally, ConnectStart and ConnectDone gets called twice, once for the resolution service and once for the actual HTTP connection. This also gets really annoying once your resolution service might be using HTTP itself, resulting in even more confusion.

Normally, I'd stash away the ClientTrace while the resolution function runs and call the DNSStart and DNSDone functions independently, but there is no way to remove a ClientTrace once it's been put into the context.

I'm fairly confident I can come up with a workaround for this, but this sort of interaction is unexpected and could cause confusion if you were using a library for ClientTraces that weren't expecting overridden Dial functions.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented May 1, 2018

@andybons andybons added this to the Unplanned milestone May 1, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 1, 2018

I'm not entirely convinced this is a bug. The Transport.DialFunc is for the Transport. If you have some unrelated dialing to do, use an unrelated context?

@DanielMorsing

This comment has been minimized.

Copy link
Contributor Author

DanielMorsing commented May 1, 2018

For the bug report, this dialing is not connected, but in the program I'm writing, we're using a network name resolution to go from hostname to IP. In that case, I want to enable cancellation via the context without getting confusing httptrace callbacks

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 1, 2018

How about:

package httptrace
func WithoutClientTrace(context.Context) context.Context { ... }
@DanielMorsing

This comment has been minimized.

Copy link
Contributor Author

DanielMorsing commented May 1, 2018

Either that or make WithClientTrace take a nil *ClientTrace to delete it from the context

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 1, 2018

That works, since nil is already reserved:

func WithClientTrace(ctx context.Context, trace *ClientTrace) context.Context {
	if trace == nil {
		panic("nil trace")
	}
...

Want to send a CL, including test & docs?

@DanielMorsing

This comment has been minimized.

Copy link
Contributor Author

DanielMorsing commented May 1, 2018

Can do, will probably send it off tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.