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: consider making Transport clean up after itself #24739

Open
FiloSottile opened this Issue Apr 6, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@FiloSottile
Member

FiloSottile commented Apr 6, 2018

If a Transport falls out of scope with idle connections, it will leak.

This is surprising for users (for example #24719), and obscure/inconsistent. os.File closes resources when falling out of scope, and Transport doesn't even have a Close method, only CloseIdleConnections. Idle connections are an implementation detail.

Implementing a finalizer would require making sure that all blocked goroutines don't keep pointers to the Transport itself, which might be hard.

Some previous discussion at #16005.

@FiloSottile FiloSottile added the Thinking label Apr 6, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Apr 6, 2018

/cc @bradfitz

@FiloSottile FiloSottile added this to the Unplanned milestone Apr 6, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Apr 6, 2018

I don't want this to be a crutch that people rely on, since finalizers are not reliable in general.

If we do use a finalizer, I'd only use it to warn.

But I don't even think it'd be possible to split Transport in two parts as @bcmills had suggested, without breaking API.

I've instead considered some (probably optional) thing that you can enable in your binary while debugging that looks at active goroutines and finds leaked Transports, perhaps using @randall77's new viewcore/heap inspection stuff, to figure out which Transports are only referencing themselves. hand wave

But given that it'd be a fair bit of work either way, and we'd only use it to warn, it's kinda low priority.

Something to consider more for Go2, perhaps.

@bcmills

This comment has been minimized.

Member

bcmills commented Apr 7, 2018

I think it would be possible to split up the object without breaking the API, but it would make the internals a bit funny. Basically, at the first RoundTrip call (and the first call after every CloseIdleConnections), we'd copy the contents of the exported fields into the internal implementation object, and refer only to the internal object from background connection goroutines. If need be, we could use an atomic variable to make the “first call” check inexpensive.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Apr 7, 2018

@bcmills, what about references back to the *Transport from func hooks configured off the *Transport?

https://golang.org/pkg/net/http/#Transport -- Proxy, Dial*, TLSNextProto

@bcmills

This comment has been minimized.

Member

bcmills commented Apr 7, 2018

They seem to mostly not be the sort of functions likely to refer back — and if they're only involved in the dialing / handshake path, they might not need to be reachable from idle-connection goroutines anyway. (That is, we might not need to copy those fields to the internal object.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment