net/http: Leaking handler function goroutines for hijacked conns

[betamos]

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

$ go version
go version go1.19.4 linux/amd64

Does this issue reproduce with the latest release?

I think so?

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/betamos/.cache/go-build"
GOENV="/home/betamos/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/betamos/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/betamos/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/betamos/code/netbench/go.mod"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4018697225=/tmp/go-build -gno-record-gcc-switches"

What did you do?

func main() {
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
                // creating ghost
		wh, _ := w.(http.Hijacker)
		_, _, _ = wh.Hijack()
		time.Sleep(10 * time.Second)
		fmt.Println("boo!")
	})
	s := &http.Server{
		Addr: ":8080",
	}
	go s.ListenAndServe()
	time.Sleep(5 * time.Second)
	s.Shutdown(context.Background())
	fmt.Println("cleared out the ghosts")
	time.Sleep(time.Hour)
}

1. Start this server
2. Run curl http://localhost:8080 within 5 seconds
3. Watch CLI output

(Apologies for using timeouts to illustrate the effect - this is certainly possible to do in a more deterministic way)

What did you expect to see?

boo!
cleared out the ghosts

What did you see instead?

cleared out the ghosts
boo!

Ok, but why?

I know that hijacked conns are not under the responsibility of http. However, there's a race condition between hijacking and passing the hijacked conn to a nicer place (perhaps for eternal resting?), which currently is managed by nobody.

Yes, I'm aware of RegisterOnShutdown. It doesn't help, because it doesn't offer any happens-before relationship w.r.t the time window mentioned above. In other words, any post-hijack handler code is not managed by anyone.

It seems to me like the handler goroutines themselves simply aren't tracked, and I think the responsibility of doing that lies within the creation of those goroutines, which is http.Server. In the (holy) spirit of structured concurrency, anything that giveth brith to a goroutine must also offer a way to await their completion.

Workarounds

Right now, the user must manually signal (through eg a waitgroup) before the attempted hijacking occurs, and then signal again upon completion, and finally await all of the outstanding ghost handlers:

// In main:
var wg sync.WaitGroup
go s.ListenAndServe()

// In handler:
wg.Add(1)
defer wg.Done()
_, _, _ = wh.Hijack()

// In main, again:
s.Shutdown()
wg.Wait()

[seankhliao]

This is working as intended. It is your responsibility to handle the goroutine and connection after a call to hijack, setting up any management beforehand as you've shown in your workaround section.

[betamos]

For other HTTP hijackers who are running across this issue in the future:

Note that this is affecting "upgraders" (like in Gorilla's WebSocket library). If you're meticulous about goroutine leaks and graceful termination, you need such a workaround. Personally, I'm upgrading-and-transferring the connections outside of the handler to take care of them elsewhere, which is more aligned with structured concurrency.