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.

Sign up for GitHub

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: conn not closed if Transport.OnProxyConnectResponse returns non-nil err #64804

Closed
wm775825 opened this issue Dec 19, 2023 · 4 comments · May be fixed by #64805
Closed

net/http: conn not closed if Transport.OnProxyConnectResponse returns non-nil err #64804

wm775825 opened this issue Dec 19, 2023 · 4 comments · May be fixed by #64805
Assignees
@neild
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@wm775825
Copy link

wm775825 commented Dec 19, 2023 •
edited

Go version

go 1.20 and 1.21, no matter what minor version or arch is.

What operating system and processor architecture are you using (go env)?

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\11749\AppData\Local\go-build
set GOENV=C:\Users\11749\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=E:\envs\go_path\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=E:\envs\go_path
set GOPRIVATE=
set GOPROXY=https://goproxy.cn,direct
set GOROOT=E:\envs\go120
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=E:\envs\go120\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\11749\AppData\Local\Temp\go-build881946686=/tmp/go-build -gno-record-gcc-switches

What did you do?

issue #54299 introduced Transport.OnProxyConnectResponse to find out how a CONNECT request to a proxy failed. (HTTP 403, etc).

So i customized my OnProxyConnectResponse to produce a non-nil error and then it returned the error.
However, there is no call to conn.Close before it returns, which is present at anywhere conn is in scope and return happens. And this may cause resource leak.

What did you expect to see?

When OnProxyConnectResponse returns with a non-nil error, conn.Close should be called before return.

What did you see instead?

When OnProxyConnectResponse returns with a non-nil error, there is no conn.Close before return.
@wm775825
Copy link
Author

Source code is here: https://github.com/golang/go/blob/master/src/net/http/transport.go#L1763

@wm775825 wm775825 changed the title net/http: conn not closed if OnProxyConnectResponse returns non-nil err net/http: conn not closed if Transport.OnProxyConnectResponse returns non-nil err Dec 19, 2023
@wm775825 wm775825 mentioned this issue Dec 19, 2023
Open
@gopherbot
Copy link

Change https://go.dev/cl/551336 mentions this issue: net/http: close connection if Transport.OnProxyConnectResponse returns non-nil err

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 19, 2023
@thanm
Copy link
Contributor

thanm commented Dec 19, 2023

Thanks for the report.

@neild per owners. Damien, unsure as to the milestone for this issue (backlog vs 1.22), let me know your thoughts.

@neild neild added this to the Go1.22 milestone Jan 4, 2024
@neild neild self-assigned this Jan 4, 2024
@neild neild added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 4, 2024
@gopherbot
Copy link

Change https://go.dev/cl/560856 mentions this issue: net/http: close connection if OnProxyConnectResponse returns an error

@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@neild @ezz-no
net/http: close connection if OnProxyConnectResponse returns an error
79f9d80 
Fixes golang#64804

Change-Id: Ibe56ab8d114b8826e477b0718470d0b9fbfef9b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/560856
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neild neild

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

Successfully merging a pull request may close this issue.

net/http: close connection if Transport.OnProxyConnectResponse returns non-nil err wm775825/go
4 participants
@neild @gopherbot @thanm @wm775825