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

net:websocket EINTR handling #22279

Merged
merged 3 commits into from
Sep 22, 2024
Merged

net:websocket EINTR handling #22279

merged 3 commits into from
Sep 22, 2024

Conversation

ismyhc
Copy link
Contributor

@ismyhc ismyhc commented Sep 22, 2024

I am working on a TUI that connections to websocket servers. I am getting premature ws closures specifically when using net.websocket client in conjunction with term.ui. By checking for EINTR error code and continuing rather than returning ws.read_next_message() fails we avoid closing prematurely. EINTR calls should not cause ws closures in this case.

This change makes the websocket behave as expected.

I hope this helps others as for me it has fixed the problem.

@JalonSolov
Copy link
Contributor

The only thing I see off the bat here is that you've "polluted" a pure V file with C code.

I would suggest either creating an EINTR const with the actual value, or create a .c.v file with the change.

The other possibility is to simply rename the current file, but that could be a breaking change if any of the other backends are using this code.

@ismyhc
Copy link
Contributor Author

ismyhc commented Sep 22, 2024

The only thing I see off the bat here is that you've "polluted" a pure V file with C code.

I would suggest either creating an EINTR const with the actual value, or create a .c.v file with the change.

The other possibility is to simply rename the current file, but that could be a breaking change if any of the other backends are using this code.

Okay, that makes sense. I've changed it to use the built in net.error_eintr const already established in net module.

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Good work.

@spytheman spytheman merged commit 3ec4939 into vlang:master Sep 22, 2024
64 of 67 checks passed
@ismyhc ismyhc deleted the ws_eintr_fix branch September 22, 2024 11:22
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.

3 participants