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

add nocopy to conn #349

Closed
piegamesde opened this issue Oct 7, 2022 · 4 comments
Closed

add nocopy to conn #349

piegamesde opened this issue Oct 7, 2022 · 4 comments
Milestone

Comments

@piegamesde
Copy link

piegamesde commented Oct 7, 2022

Stack trace:

go.ts:72 panic: runtime error: index out of range [0] with length 0
go.ts:72 
go.ts:72 goroutine 15 [running]:
go.ts:72 nhooyr.io/websocket.(*Conn).read(0x4b8000, 0xa1238, 0x438b00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
go.ts:72 	/root/go/pkg/mod/nhooyr.io/websocket@v1.8.7/ws_js.go:132 +0x35
go.ts:72 nhooyr.io/websocket.(*Conn).Read(0x4b8000, 0xa1238, 0x438b00, 0x0, 0x2a0108, 0x18, 0x9eed8, 0x1b920004, 0x4b8000)
go.ts:72 	/root/go/pkg/mod/nhooyr.io/websocket@v1.8.7/ws_js.go:107 +0xb

At the time where the panic happens, the message that is about to be read has already been received by the browser.

I looked at the relevant code and it looks a bit weird at first but could not find any obvious bug causing this.

@piegamesde
Copy link
Author

Semi-related: If I understand the code correctly, it is using a single-element channel and a buffer plus mutex in order to create an unbounded channel. This means that no backpressure will be applied to the sender if the receiver is not fast enough at processing the messages. I think this should be changed.

@piegamesde
Copy link
Author

It turns out that we were storing the Conn struct by value, which caused some issues with Mutexes getting copied. Switching to pointers trivially fixed the issue: https://github.com/LeastAuthority/wormhole-william/pull/91/files

However, are there any ways to ensure this does not happen again? Can we somehow make sure that Conn is copy-safe, or alternatively that it is not copied accidentally? At the very least, I think this should be documented.

@nhooyr
Copy link
Owner

nhooyr commented Mar 5, 2023

However, are there any ways to ensure this does not happen again? Can we somehow make sure that Conn is copy-safe, or alternatively that it is not copied accidentally? At the very least, I think this should be documented.

go vet should have flagged this for you. Or perhaps I need to add something here to make it do so.

It is already documented, that's why the receiver on all Conn methods is a pointer and why a pointer is returned. You can't go from pointer -> value and expect things to work.

Yup I'll add golang/go#8005 (comment)

@nhooyr nhooyr changed the title Index out of bounds panic on WASM add nocopy to conn Mar 5, 2023
@nhooyr nhooyr added this to the v1.8.8 milestone Sep 28, 2023
nhooyr added a commit that referenced this issue Oct 13, 2023
@nhooyr
Copy link
Owner

nhooyr commented Oct 13, 2023

Fixed in dev.

@nhooyr nhooyr closed this as completed Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants