websocket icon indicating copy to clipboard operation
websocket copied to clipboard

Deflake proxy unit tests

Open liggitt opened this issue 8 months ago • 4 comments

What type of PR is this? (check all applicable)

  • [ ] Bug Fix

This PR is best viewed ignoring whitespace (a couple functions just got indented) - https://github.com/gorilla/websocket/pull/982/files?w=1

Description

I noticed a flake on main unit tests at https://app.circleci.com/pipelines/github/gorilla/websocket/424/workflows/e37b2052-9796-4d0e-b3f6-9c6f72980e3f/jobs/1529?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link&utm_content=summary

Dug in and found a few tweaks needed in the unit tests from https://github.com/gorilla/websocket/pull/978

This PR:

  1. Correctly closes the test proxy server
  2. Correctly handles any buffered data when hijacking the connection in the test handler
  3. Fixes a panic if the upgrade failed inside the test websocket handler (was happening due to not handling the buffered data)
  4. Switches from calling http.Error after Upgrade (which doesn't work) to logging errors to the test logger

Added/updated tests?

  • [x] Yes, fixed unit test flakes

Run verifications and test

go test -race ./...
ok  	github.com/gorilla/websocket	2.616s
?   	github.com/gorilla/websocket/examples/autobahn	[no test files]
?   	github.com/gorilla/websocket/examples/chat	[no test files]
?   	github.com/gorilla/websocket/examples/command	[no test files]
?   	github.com/gorilla/websocket/examples/filewatch	[no test files]

On main:

$ go test -race -c .
$ stress ./websocket.test 

/var/folders/37/ns7gt60104scfm9fvg02p1jh00kjgb/T/go-stress-20250320T105828-994893878
2025/03/20 10:58:28 http: superfluous response.WriteHeader call from github.com/gorilla/websocket.init.func2 (client_proxy_server_test.go:640)
2025/03/20 10:58:28 http: panic serving 127.0.0.1:55001: runtime error: invalid memory address or nil pointer dereference
goroutine 15 [running]:
net/http.(*conn).serve.func1()
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/net/http/server.go:1947 +0xe4
panic({0x104d71fa0?, 0x1050a4500?})
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/runtime/panic.go:787 +0x124
github.com/gorilla/websocket.(*Conn).Close(...)
	/Users/liggitt/go/src/github.com/gorilla/websocket/conn.go:344
panic({0x104d71fa0?, 0x1050a4500?})
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/runtime/panic.go:787 +0x124
github.com/gorilla/websocket.(*Conn).beginMessage(0x0, 0xc0000fecc0, 0x2)
	/Users/liggitt/go/src/github.com/gorilla/websocket/conn.go:488 +0x30
github.com/gorilla/websocket.(*Conn).NextWriter(0x0, 0x2)
	/Users/liggitt/go/src/github.com/gorilla/websocket/conn.go:529 +0x78
github.com/gorilla/websocket.init.func2({0x104ded610, 0xc0002860e0}, 0xc0000f4640)
	/Users/liggitt/go/src/github.com/gorilla/websocket/client_proxy_server_test.go:644 +0x1b4
net/http.HandlerFunc.ServeHTTP(0x104de5880, {0x104ded610, 0xc0002860e0}, 0xc0000f4640)
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/net/http/server.go:2294 +0x4c
net/http.serverHandler.ServeHTTP({0xc0000fea50?}, {0x104ded610, 0xc0002860e0}, 0xc0000f4640)
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/net/http/server.go:3301 +0x29c
net/http.(*conn).serve(0xc0000d4990, {0x104ded928, 0xc0000fe930})
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/net/http/server.go:2102 +0xeb8
created by net/http.(*Server).Serve in goroutine 37
	/Users/liggitt/.gimme/versions/go1.24.0.darwin.arm64/src/net/http/server.go:3454 +0x678
--- FAIL: TestHTTPProxyWithNetDialContext (0.00s)
    client_proxy_server_test.go:151: websocket dial error: unexpected EOF
2025/03/20 10:58:29 http: TLS handshake error from 127.0.0.1:
…
1m0s: 217 runs so far, 6 failures (2.76%), 10 active

With this PR:

$ go test -race -c .
$ stress ./websocket.test 
5s: 10 runs so far, 0 failures, 10 active
10s: 30 runs so far, 0 failures, 10 active
15s: 48 runs so far, 0 failures, 10 active
20s: 66 runs so far, 0 failures, 10 active
25s: 83 runs so far, 0 failures, 10 active
30s: 93 runs so far, 0 failures, 10 active
35s: 111 runs so far, 0 failures, 10 active
40s: 131 runs so far, 0 failures, 10 active
45s: 151 runs so far, 0 failures, 10 active
50s: 169 runs so far, 0 failures, 10 active
55s: 187 runs so far, 0 failures, 10 active
1m0s: 205 runs so far, 0 failures, 10 active
1m5s: 223 runs so far, 0 failures, 10 active
1m10s: 241 runs so far, 0 failures, 10 active
1m15s: 254 runs so far, 0 failures, 10 active
1m20s: 268 runs so far, 0 failures, 10 active
1m25s: 286 runs so far, 0 failures, 10 active
1m30s: 304 runs so far, 0 failures, 10 active
1m35s: 322 runs so far, 0 failures, 10 active
1m40s: 340 runs so far, 0 failures, 10 active
1m45s: 358 runs so far, 0 failures, 10 active
1m50s: 376 runs so far, 0 failures, 10 active
1m55s: 394 runs so far, 0 failures, 10 active
2m0s: 412 runs so far, 0 failures, 10 active
2m5s: 435 runs so far, 0 failures, 10 active
2m10s: 455 runs so far, 0 failures, 10 active
2m15s: 474 runs so far, 0 failures, 10 active
2m20s: 492 runs so far, 0 failures, 10 active
2m25s: 510 runs so far, 0 failures, 10 active
2m30s: 528 runs so far, 0 failures, 10 active
...

cc @seans3 @aojea

liggitt avatar Mar 20 '25 14:03 liggitt

cc @AlexVulaj

liggitt avatar Mar 20 '25 15:03 liggitt

/lgtm

aojea avatar Mar 20 '25 15:03 aojea

/lgtm

Thanks for the clean-up and fixes.

seans3 avatar Mar 20 '25 23:03 seans3

@AlexVulaj friendly ping on this as a follow-up to https://github.com/gorilla/websocket/pull/978

liggitt avatar May 09 '25 15:05 liggitt