Navigation Menu

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

Jump to bottom

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

Open
DanielMorsing opened this issue May 1, 2018 · 8 comments
Open

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

DanielMorsing opened this issue May 1, 2018 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@DanielMorsing
Copy link
Contributor

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
Copy link
Member

andybons commented May 1, 2018

@bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2018
@andybons andybons added this to the Unplanned milestone May 1, 2018
@bradfitz
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

bradfitz commented May 1, 2018

How about:

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

@DanielMorsing
Copy link
Contributor Author

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

@bradfitz
Copy link
Contributor

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
Copy link
Contributor Author

Can do, will probably send it off tomorrow

@frankli0324
Copy link

I encountered this and found this still open, so I have no choice but implementing a hack:

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

var stdNetTraceKey, stdHttpTraceKey interface{}

type captureContext struct {
	context.Context
	capture func(reflect.Type)
}

func (c captureContext) Value(key interface{}) interface{} {
	c.capture(reflect.TypeOf(key))
	return nil
}

func init() {
	var stdNetTraceType, stdHttpTraceType reflect.Type

	capture := captureContext{context.Background(), nil}
	capture.capture = func(t reflect.Type) { stdNetTraceType = t }
	(&net.Dialer{}).DialContext(capture, "invalid", "")
	capture.capture = func(t reflect.Type) { stdHttpTraceType = t }
	httptrace.ContextClientTrace(capture)

	stdNetTraceKey = reflect.New(stdNetTraceType).Elem().Interface()
	stdHttpTraceKey = reflect.New(stdHttpTraceType).Elem().Interface()
}

func shadowStandardClientTrace(ctx context.Context) context.Context {
	ctx = context.WithValue(ctx, stdHttpTraceKey, nil)
	ctx = context.WithValue(ctx, stdNetTraceKey, nil)
	return ctx
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @andybons @DanielMorsing @frankli0324