Skip to content

fix: handle return value of uv_pipe_connect2 #779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 28, 2025
Merged

fix: handle return value of uv_pipe_connect2 #779

merged 1 commit into from
Jul 28, 2025

Conversation

seandewar
Copy link
Contributor

@seandewar seandewar commented Jul 28, 2025 •
edited
Loading

The missing check meant that no error was propagated when it returned UV_EINVAL.
This notably produced no error when specifying an invalid name, and made specifying UV_PIPE_NO_TRUNCATE ineffective.

This was likely missed out because libuv's documented return value for uv_pipe_connect2 incorrectly stated that it returns void, not int; that's since been fixed. (libuv/libuv#4847)

@seandewar
fix: handle return value of uv_pipe_connect2
3995fc4 
The missing check meant that no error was propagated when it returned UV_EINVAL.
This notably produced no error when specifying an invalid name, and made
specifying UV_PIPE_NO_TRUNCATE ineffective.

This was likely missed out because libuv's documented return value for
uv_pipe_connect2 incorrectly stated that it returns void, not int; that's since
been fixed.
@squeek502
Copy link
Member

squeek502 commented Jul 28, 2025 •
edited
Loading

Thanks! From looking at the libuv source, uv_pipe_connect just calls uv_pipe_connect2, so it can also error (not via UV_PIPE_NO_TRUNCATE, but in all the other possible ways).

Mind making the same change to luv_pipe_connect?

EDIT: Sorry, uv_pipe_connect actually has a void return type, so nevermind.

@seandewar
Copy link
Contributor Author

seandewar commented Jul 28, 2025 •
edited
Loading

uv_pipe_connect returns void, so it instead sends the error to the callback. (I think that means luv would see it on the next event-loop tick?) Ah, I missed the edit. 😁

@squeek502
Copy link
Member

You're absolute right, my mistake.

@squeek502 squeek502 enabled auto-merge (rebase) July 28, 2025 11:16
@squeek502 squeek502 merged commit fcbfdc0 into luvit:master Jul 28, 2025
15 checks passed
@seandewar seandewar deleted the connect2_rv branch July 28, 2025 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeek502 squeek502 squeek502 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@seandewar @squeek502