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/httputil: ReverseProxy - websocket connections cannot be canceled #35559

Closed
piec opened this issue Nov 13, 2019 · 5 comments
Closed

net/http/httputil: ReverseProxy - websocket connections cannot be canceled #35559

piec opened this issue Nov 13, 2019 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.15

Comments

@piec
Copy link
Contributor

piec commented Nov 13, 2019

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

$ go version
go version go1.13.4 linux/amd64

(also checked master)

Does this issue reproduce with the latest release?

Yes

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

Up to date Arch Linux amd64

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/pierre/.cache/go-build"
GOENV="/home/pierre/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pierre/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build415071953=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  • I used httputil.NewSingleHostReverseProxy to create a proxy to a backend server which serves websockets.
  • I made a request to a backend websocket service through the proxy. The request is cancelable using context.WithCancel(...).
  • I canceled the request

What did you expect to see?

I expected the reverse proxy to close the connection to the backend server.

What did you see instead?

The connection between the reverse proxy and the backend server was preserved.

Hi All,

I quickly modified the TestReverseProxyWebSocket test to reproduce my issue eshard@73147b8 (I modified TestReverseProxyWebSocket in place to make the diff more readable)
I also did a dirty fix: eshard@98e746e which propagates the cancelation to the handleUpgradeResponse function. It creates an additional goroutine so it's not ideal.

I'd be happy to improve my patch with your inputs

Best regards
Pierre
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 13, 2019
@andybons andybons added this to the Unplanned milestone Nov 13, 2019
@andybons
Copy link
Member

@bradfitz @mikioh

@bradfitz bradfitz modified the milestones: Unplanned, Go1.15 Nov 13, 2019
@odeke-em
Copy link
Member

Thank you for reporting this issue @piec and welcome to the Go project!

Apologies foe the late reply, and thanks for the patience.
Please go ahead and send a CL/PR and I’ll review it.

@piec piec mentioned this issue Mar 23, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/224897 mentions this issue: net/http/httputil: make handleUpgradeResponse cancelable

@odeke-em
Copy link
Member

odeke-em commented Apr 1, 2020 •
edited

@piec please take a look at the feedback that I left on your CL https://go-review.googlesource.com/c/go/+/224897#message-f3370368c9bfcf1cf91be57337ec9689b9a799ae, it would be great to get this in before the code freeze which is in 4 weeks. Thank you!

@piec
Copy link
Contributor Author

piec commented Apr 1, 2020

Yes I took a look at the review @odeke-em, and thank you for it.
I didn't get time to work on it yet but I'll do it in the coming days.
Cheers

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
@piec @xujianhai666
net/http/httputil: make Switching Protocol requests (e.g. Websockets)…
bffdf33 
… cancelable

Ensures that a canceled client request for Switching Protocols
(e.g. h2c, Websockets) will cause the underlying connection to
be terminated.

Adds a goroutine in handleUpgradeResponse in order to select on
the incoming client request's context and appropriately cancel it.

Fixes golang#35559

Change-Id: I1238e18fd4cce457f034f78d9cdce0e7f93b8bf6
GitHub-Last-Rev: 3629c78
GitHub-Pull-Request: golang#38021
Reviewed-on: https://go-review.googlesource.com/c/go/+/224897
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@gernest gernest mentioned this issue Aug 17, 2020
Merged
17 tasks
@golang golang locked and limited conversation to collaborators Apr 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

net/http/httputil: make Switching Protocol requests (e.g. Websockets) cancelable eshard/go
5 participants
@bradfitz @andybons @piec @odeke-em @gopherbot