Skip to content

Deflake proxy unit tests #982

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Deflake proxy unit tests #982

wants to merge 1 commit into from

Conversation

liggitt
Copy link

@liggitt liggitt commented Mar 20, 2025

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 #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?

  • 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
Copy link
Author

liggitt commented Mar 20, 2025

cc @AlexVulaj

@aojea
Copy link

aojea commented Mar 20, 2025

/lgtm

@seans3
Copy link
Contributor

seans3 commented Mar 20, 2025

/lgtm

Thanks for the clean-up and fixes.

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

Successfully merging this pull request may close these issues.

3 participants