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

Fixed empty readBuf issue #91

Merged
merged 1 commit into from Oct 25, 2022

Conversation

donpui
Copy link

@donpui donpui commented Oct 14, 2022

Due to double pointers (my guess) misbehaviour happen in nhooyr/websocket ws_js.go library.
On receiver side, when wsMessage appear, it was appended to c.readBuf
https://github.com/nhooyr/websocket/blob/8dee580a7f74cf1713400307b4eee514b927870f/ws_js.go#L81
and signal created: case c.readSignal <- struct{}{}:

Then in read function, when signal appear, we tried to read from the same c.readBuf, however it was empty and we got go panic:
panic: runtime error: index out of range [0] with length 0 (nhooyr/websocket#349)
https://github.com/nhooyr/websocket/blob/8dee580a7f74cf1713400307b4eee514b927870f/ws_js.go#L132

c.readBuf in init() and read() functions had different memory addresses.

Changing *wsconn -> wsconn, which itself is pointer, allowed to solve an issue, without any changes on nhooyr/websocket library.


Code Review Checklist (to be filled out by reviewer)

  • Description accurately reflects what changes are being made.
  • Description explains why the changes are being made (or references an issue containing one).
  • The PR appropriately sized.
  • New code has enough tests.
  • New code has enough documentation to answer "how do I use it?" and "what does it do?".
  • Existing documentation is up-to-date, if impacted.

@meejah
Copy link
Collaborator

meejah commented Oct 14, 2022

Is there something describing the issue?

@donpui
Copy link
Author

donpui commented Oct 14, 2022

Add in description, as it was quick PR for others to try

@piegamesde
Copy link
Collaborator

I'm uneasy with calling it a day on this issue yet. It sounds like a ticking time bomb, something waiting to come back and steal us yet another few weeks after the next refactoring. We really should try and get a deeper understanding of the underlying issue.

@piegamesde piegamesde merged commit 5802757 into piegames/noise-transit Oct 25, 2022
@piegamesde piegamesde deleted the donpui/noise-transit-fix branch October 25, 2022 12:58
@donpui
Copy link
Author

donpui commented Nov 17, 2022

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