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: switch back to the gorilla library #2280

Merged
merged 4 commits into from
May 11, 2023
Merged

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented May 9, 2023

Turns out that @Jorropo was right in #1982 (comment). Switching to nyhoor's websocket library has caused us nothing but pain (examples: #1982, #2201, and most recently, ugly workaround in the gating tests).

Although the author claims to fix critical issues, nothing is happening when actual issues are reported (examples: coder/websocket#367, coder/websocket#355 and coder/websocket#384), and there hasn't even been a patch release April 2021).

This PR reverts the changes made to the WebSocket transport in reverse order they were made:

After that, it reapplies the one fix that was unrelated to the library used: #2199.

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

No strong opinion here.
I agree fixing the 5s stall alone is worth the revert.

n, err := c.Conn.Read(b)
if err == nil && n == 0 && c.readAttempts < maxReadAttempts {
c.readAttempts++
// Nothing happened, let's read again. We reached the end of the frame
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just flagging that we're losing this comment, but keeping this behavior. Maybe a similar comment would be helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we? This was introduced as part of #1982, wasn't it?

@nhooyr
Copy link

nhooyr commented Oct 19, 2023

Sorry you had a bad experience with my library. My delay in dealing with issues was unfortunate and I do sincerely apologize. I just wanted to let you know that all the issues you cited are now fixed for v1.8.8 which will be released tomorrow at noon. coder/websocket#256

Your feedback was very much appreciated even if I was too late to get things in for you.

@Jorropo
Copy link
Contributor

Jorropo commented Oct 19, 2023

@nhooyr I see you handled:

examples: coder/websocket#367, coder/websocket#355 and coder/websocket#384

Thx that is great, however my opinion is still the same, while gorilla is working (no bug or security issue known) I don't think go-libp2p should try switching away from it. However I'm not a maintainer on go-libp2p.

@nhooyr
Copy link

nhooyr commented Oct 19, 2023

By all means. I would suggest the same. The WebSocket protocol is stable and so gorilla is fine now and will be fine for a long time. I just wanted to note that the issues cited are now fixed.

@paralin
Copy link
Contributor

paralin commented Oct 19, 2023

I like the WebAssembly support in @nhooyr library, among other things. It would be worthwhile to have a look at including it, imo

btwiuse pushed a commit to btwiuse/wsport that referenced this pull request May 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

Successfully merging this pull request may close these issues.

websocket: closing a connection may block for 5s
5 participants