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

websocket.Conn.CloseNow() panics in WaitGroup.Wait #437

Closed
tomasji opened this issue Feb 26, 2024 · 2 comments
Closed

websocket.Conn.CloseNow() panics in WaitGroup.Wait #437

tomasji opened this issue Feb 26, 2024 · 2 comments
Milestone

Comments

@tomasji
Copy link

tomasji commented Feb 26, 2024

nhooyr.io/websocket v1.8.10

When CloseNow() is called by 2 goroutines, there is a race condition:

panic: sync: WaitGroup is reused before previous Wait has returned

goroutine 105321 [running]:
sync.(*WaitGroup).Wait(0xe33400?)
	/src/sync/waitgroup.go:118 +0x74
nhooyr.io/websocket.(*Conn).CloseNow(0xc02d1e5860)
	/.go/pkg/mod/nhooyr.io/websocket@v1.8.10/close.go:113 +0xcb

Original func

func (c *Conn) CloseNow() (err error) {
	defer c.wg.Wait()
	defer errd.Wrap(&err, "failed to close WebSocket")

	if c.isClosed() {
		return net.ErrClosed
	}
...
}

Possible fix: move defer calls below isClosed()

func (c *Conn) CloseNow() (err error) {
	if c.isClosed() {
		return net.ErrClosed
	}
	defer c.wg.Wait()
	defer errd.Wrap(&err, "failed to close WebSocket")
...
}
nhooyr added a commit to alixander/websocket that referenced this issue Apr 5, 2024
Far simpler now. Sorry this took a while.

Closes coder#427
Closes coder#429
Closes coder#434
Closes coder#436
Closes coder#437
@nhooyr
Copy link
Contributor

nhooyr commented Apr 5, 2024

Thanks for reporting. I've fixed this in #427. Please review and let me know if you see anything else.

nhooyr added a commit to alixander/websocket that referenced this issue Apr 5, 2024
Far simpler now. Sorry this took a while.

Closes coder#427
Closes coder#429
Closes coder#434
Closes coder#436
Closes coder#437
@nhooyr nhooyr added this to the v1.8.11 milestone Apr 5, 2024
@nhooyr
Copy link
Contributor

nhooyr commented Apr 7, 2024

Sorry for the delay. Fixed by #427.

@nhooyr nhooyr closed this as completed Apr 7, 2024
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

No branches or pull requests

2 participants