GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets by ggoodman · Pull Request #49667 · nodejs/node

Conversation

ggoodman commented Sep 15, 2023

Copy link

Copy Markdown

Contributor

This PR moves PipeWrap from the classic uv_pipe_bind and uv_pipe_connect methods to their newer uv_pipe_bind2 and uv_pipe_connect2 counterparts. This restores support for connecting to abstract unix domain sockets. Support was lost due to the refactor in libuv/libuv#4030 where uv_pipe_bind and uv_pipe_connect now infer the length of the pathname using strlen(). For an abstract unix socket, this was producing 0. Moving to uv_pipe_bind2 and uv_pipe_connect2 allows us to pass an explicit size_t for the socket path based on the length of the v8 string.

Fixes: #49656

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji

All reactions

👀 1 reaction

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci

PRs that need a full CI run.

net Issues and PRs related to the net subsystem. labels

Sep 15, 2023

ggoodman commented Sep 15, 2023

Copy link

Copy Markdown

Contributor Author

Alright, here we go @santigimeno. I think I got the commit message guidelines down. Looking for feedback on the test. It should do the thing but might not be at the peak of elegance or codebase best practices.

Also, since I'm expecting to make changes; is rebasing ok or do we prefer new fix commits for review purposes?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

santigimeno requested changes

Sep 15, 2023

View reviewed changes

santigimeno left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Mostly looking good. Left some comments. Also a good idea is for you to test if the linter passes: make lint.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

test/parallel/test-pipe-abstract-socket-http.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

test/parallel/test-pipe-abstract-socket-http.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

santigimeno reviewed

Sep 15, 2023

View reviewed changes

Comment thread

test/parallel/test-pipe-abstract-socket-http.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ggoodman commented Sep 15, 2023

Copy link

Copy Markdown

Contributor Author

@santigimeno turns out the guide you linked has almost exactly the test I wanted. I added an extra common.mustCall to ensure that the server successfully binds and calls the "listening" callback. Looks nice and tight now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Sep 15, 2023

View reviewed changes

lpinca left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGMT with lint issues fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ggoodman force-pushed the fix-49656 branch from 2db02d2 to 420c22b Compare

September 15, 2023 19:13

santigimeno approved these changes

Sep 15, 2023

View reviewed changes

santigimeno left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Great work!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ggoodman commented Sep 15, 2023

Copy link

Copy Markdown

Contributor Author

Ugh, copy paste mistake. Fixing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ggoodman added 2 commits

September 15, 2023 15:38


          src: port Pipe to uv_pipe_bind2, uv_pipe_connect2

85e27ed

The introduction of the uv_pipe_bind2 and uv_pipe_connect2 methods in
libuv v1.46.0 changed the behaviour of uv_pipe_bind and uv_pipe_connect.
This broke the ability to connect to abstract domain sockets on linux.
This change ports PipeWrap to use the new uv_pipe_bind2 and
uv_pipe_connect2 methods to restore abstract domain socket support.

Fixes: nodejs#49656
Refs: libuv/libuv#4030


          test: adds linux-only test for abstract sockets

8afecf1

Introduce a new linux-only test for binding to an abstract unix socket
and then making an http request against that socket.

Refs: nodejs#49656

ggoodman force-pushed the fix-49656 branch from 420c22b to 8afecf1 Compare

September 15, 2023 19:39

debadree25 added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Sep 15, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Sep 15, 2023

nodejs-github-bot commented Sep 15, 2023

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53971/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ggoodman commented Sep 15, 2023

Copy link

Copy Markdown

Contributor Author

Seems like the osx11-x64 configuration in CI is having a bad day. Is the CI failure related or unrelated?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Sep 16, 2023

CI Reliability 2023-09-16 nodejs/reliability#667

Open

25 tasks

debadree25 commented Sep 16, 2023

Copy link

Copy Markdown

Contributor

Looks unrelated will restart ci

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Sep 16, 2023

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53983/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Sep 16, 2023

View reviewed changes

This was referenced Sep 17, 2023

CI Reliability 2023-09-17 nodejs/reliability#668

Open

CI Reliability 2023-09-18 nodejs/reliability#669

Open

debadree25 added commit-queue

PRs queued for automated landing through the Commit Queue.

commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels

Sep 18, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Sep 18, 2023

nodejs-github-bot merged commit 553169f into nodejs:main

Sep 18, 2023

nodejs-github-bot commented Sep 18, 2023

Copy link

Copy Markdown

Collaborator

Landed in 553169f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

debadree25 commented Sep 18, 2023

Copy link

Copy Markdown

Contributor

Thank you for your contribution @ggoodman 🚀

ggoodman reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Sep 19, 2023

CI Reliability 2023-09-19 nodejs/reliability#670

Open

CI Reliability 2023-09-20 nodejs/reliability#671

Open

ruyadorno pushed a commit that referenced this pull request

Sep 28, 2023


          src: port Pipe to uv_pipe_bind2, uv_pipe_connect2

a597cb8

The introduction of the uv_pipe_bind2 and uv_pipe_connect2 methods in
libuv v1.46.0 changed the behaviour of uv_pipe_bind and uv_pipe_connect.
This broke the ability to connect to abstract domain sockets on linux.
This change ports PipeWrap to use the new uv_pipe_bind2 and
uv_pipe_connect2 methods to restore abstract domain socket support.

Fixes: #49656
Refs: libuv/libuv#4030
PR-URL: #49667
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

deokjinkim added a commit to deokjinkim/node that referenced this pull request

Nov 10, 2023


          src: handle errors from uv_pipe_connect2()

68ce7f9

We need to handle errors from uv_pipe_connect2()
because return type is `int`.

Fixes: nodejs#50652
Refs: nodejs#49667
Refs: libuv/libuv#4030

deokjinkim mentioned this pull request

Nov 10, 2023

src: handle errors from uv_pipe_connect2() #50657

Merged

nodejs-github-bot pushed a commit that referenced this pull request

Nov 12, 2023


          src: handle errors from uv_pipe_connect2()

9134f4a

We need to handle errors from uv_pipe_connect2()
because return type is `int`.

Fixes: #50652
Refs: #49667
Refs: libuv/libuv#4030
PR-URL: #50657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: theanarkh <[email protected]>

targos pushed a commit that referenced this pull request

Nov 23, 2023


          src: handle errors from uv_pipe_connect2()

57587de

We need to handle errors from uv_pipe_connect2()
because return type is `int`.

Fixes: #50652
Refs: #49667
Refs: libuv/libuv#4030
PR-URL: #50657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: theanarkh <[email protected]>

yseymour mentioned this pull request

Nov 23, 2023

When binding server to a UNIX socket in an abstract namespace, the address length is wrong #50889

Closed

theanarkh mentioned this pull request

Nov 25, 2023

doc: add doc for Unix abstract socket #50904

Merged

4 tasks done

nodejs-github-bot pushed a commit that referenced this pull request

Nov 28, 2023


          doc: add doc for Unix abstract socket

69866bf

PR-URL: #50904
Refs: #49667
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

targos pushed a commit that referenced this pull request

Dec 4, 2023


          doc: add doc for Unix abstract socket

eecab88

PR-URL: #50904
Refs: #49667
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

UlisesGascon pushed a commit that referenced this pull request

Dec 11, 2023


          src: handle errors from uv_pipe_connect2()

c253e39

We need to handle errors from uv_pipe_connect2()
because return type is `int`.

Fixes: #50652
Refs: #49667
Refs: libuv/libuv#4030
PR-URL: #50657
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: theanarkh <[email protected]>

richardlau pushed a commit that referenced this pull request

Mar 25, 2024


          doc: add doc for Unix abstract socket

5f8e7a0

PR-URL: #50904
Refs: #49667
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

Reviewers

jasnell

jasnell approved these changes

santigimeno

santigimeno approved these changes

lpinca

lpinca approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

needs-ci

PRs that need a full CI run.

net

Issues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Upgrade to libuv version 1.46.0 broke connecting to abstract unix sockets

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.