Skip to content

src: add UV_PIPE_NO_TRUNCATE for bind in pipe_wrap.cc#52347

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:add_UV_PIPE_NO_TRUNCATE_for_pipe
May 6, 2024
Merged

src: add UV_PIPE_NO_TRUNCATE for bind in pipe_wrap.cc#52347
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:add_UV_PIPE_NO_TRUNCATE_for_pipe

Conversation

@theanarkh

Copy link
Copy Markdown
Contributor

Binding a pipe path that is too long can cause problems and confuse the user.

example1:

const net = require('net');
const fs = require('fs');

const filePath = `/tmp/${'x'.repeat(1000)}.sock`;
const server = net.createServer()
    // the path will be truncated
    .listen(filePath)
    .on('listening', () => {
        // here will output true but the socket is not actually bound to filePath
        console.log(server.address() === filePath);
        // here will output false because the file do not exist
        console.log(fs.existsSync(filePath))
    })

example2:

const net = require('net');

const filePath1 = `/tmp/${'x'.repeat(1001)}.sock`;
const filePath2 = `/tmp/${'x'.repeat(1002)}.sock`;
// listen successfully
net.createServer().listen(filePath1);
// here will fail with EADDRINUSE error
net.createServer().listen(filePath2);

I think it is better to throw an error when the pipe path is truncated. But it maybe a breaking change.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot 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 Apr 3, 2024
@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from f2d4433 to 92f36e7 Compare April 3, 2024 15:04
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from 92f36e7 to 85adc9e Compare April 3, 2024 20:31
Comment thread doc/api/net.md Outdated
@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from 85adc9e to fb5aa12 Compare April 4, 2024 08:47
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@daeyeon daeyeon added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 19, 2024

@daeyeon daeyeon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is definitely a breaking change. As suggested, I think it would be better to throw an error in this case to make users aware of potential problems.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from fb5aa12 to d798da4 Compare April 21, 2024 15:56
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

nodejs-github-bot commented Apr 23, 2024

Copy link
Copy Markdown
Collaborator

@theanarkh theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2024
@jasnell

jasnell commented Apr 27, 2024

Copy link
Copy Markdown
Member

@nodejs/tsc ... this needs another TSC review.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for opening a PR! Can you please add a unit test?

@daeyeon

daeyeon commented Apr 28, 2024

Copy link
Copy Markdown
Member

There was a test, but it seems to be accidentally deleted. Please add it again.

@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from d798da4 to c3901ad Compare April 29, 2024 15:44

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@theanarkh theanarkh force-pushed the add_UV_PIPE_NO_TRUNCATE_for_pipe branch from c3901ad to 4e06896 Compare May 3, 2024 17:55
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels May 6, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7588467 into nodejs:main May 6, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 7588467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants