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

fix: Use sync.WaitGroup to await hijacked HTTP connections #337

Merged
merged 2 commits into from
Feb 20, 2022
Merged

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Feb 20, 2022

WebSockets hijack the HTTP connection from the server, causing
server.Close() to not wait for these connections to fully cleanup.

This adds a global wait-group to the coderd API, which ensures all
WebSocket HTTP handlers have properly exited before returning.

Closes #317

@kylecarbs kylecarbs self-assigned this Feb 20, 2022
@codecov
Copy link

codecov bot commented Feb 20, 2022

Codecov Report

Merging #337 (bbd23dd) into main (3c04c7f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
- Coverage   67.61%   67.58%   -0.03%     
==========================================
  Files         142      142              
  Lines        7700     7706       +6     
  Branches       77       77              
==========================================
+ Hits         5206     5208       +2     
- Misses       1966     1968       +2     
- Partials      528      530       +2     
Flag Coverage Δ
unittest-go-macos-latest 66.31% <100.00%> (-0.07%) ⬇️
unittest-go-ubuntu-latest 67.51% <100.00%> (+0.19%) ⬆️
unittest-go-windows-2022 65.93% <100.00%> (+0.06%) ⬆️
unittest-js 63.61% <ø> (ø)
Impacted Files Coverage Δ
coderd/cmd/root.go 81.94% <100.00%> (+0.25%) ⬆️
coderd/coderd.go 95.19% <100.00%> (-0.10%) ⬇️
coderd/coderdtest/coderdtest.go 100.00% <100.00%> (ø)
coderd/provisionerdaemons.go 57.66% <100.00%> (-0.18%) ⬇️
cli/login.go 60.46% <0.00%> (-0.59%) ⬇️
peer/conn.go 78.51% <0.00%> (-0.52%) ⬇️
peerbroker/listen.go 84.80% <0.00%> (+1.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c04c7f...bbd23dd. Read the comment docs.

@kylecarbs kylecarbs changed the title fix: Use forked nhooyr/websocket to resolve close goroutine leak fix: Defer close of nhooyr/websocket to wait for exit Feb 20, 2022
Copy link
Contributor

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda concerning to me that we're using an abandoned library, but here's not really other options (gorilla is also abandoned)

WebSockets hijack the HTTP connection from the server, causing
server.Close() to not wait for these connections to fully cleanup.

This adds a global wait-group to the coderd API, which ensures all
WebSocket HTTP handlers have properly exited before returning.
@kylecarbs kylecarbs changed the title fix: Defer close of nhooyr/websocket to wait for exit fix: Use sync.WaitGroup to await hijacked HTTP connections Feb 20, 2022
@kylecarbs
Copy link
Member Author

This wasn't actually a problem with the library, but an interesting quirk with the Go HTTP library. Check the diff now!

@kylecarbs kylecarbs merged commit d04570a into main Feb 20, 2022
@kylecarbs kylecarbs deleted the wsleak branch February 20, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix websocket/yamux/drpc goleak failure on Windows
2 participants