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

Open
piec opened this issue Nov 13, 2019 · 5 comments · May be fixed by #38021
Open

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

piec opened this issue Nov 13, 2019 · 5 comments · May be fixed by #38021
Milestone

Comments

@piec
Copy link

@piec 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 this to the Unplanned milestone Nov 13, 2019
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Nov 13, 2019

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

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 21, 2020

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 23, 2020

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

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Apr 1, 2020

@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

This comment has been minimized.

Copy link
Author

@piec 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.