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

test: Added relative path to accommodate limit #12601

Closed

Conversation

@coreybeaumont
Copy link
Contributor

commented Apr 22, 2017

Found that libuv had a path character limit of 108. Used path.relative() to set prefix with relative path.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
test: Added relative path to accommodate limit
Found that libuv had a path character limit of 108. Used path.relative() to set prefix with relative path.
@Trott
Trott approved these changes Apr 22, 2017
Copy link
Member

left a comment

LGTM if CI is green. Thanks for fixing this!

@Trott

This comment has been minimized.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

I think there should be a comment above the change the describes why a relative path is being used (including libuv error and platform (if error is specific to say Windows filesystems or something)).

@mscdex mscdex added the net label Apr 22, 2017

@@ -32,7 +33,8 @@ const forAllClients = (cb) => common.mustCall(cb, CLIENT_VARIANTS);

// Test Pipe fd is wrapped correctly
{
const prefix = `${common.PIPE}-net-connect-options-fd`;
const prefix = path.relative(`${common.PIPE}-net-connect-options-fd`,
`${__dirname}-net-connect-options-fd`);

This comment has been minimized.

Copy link
@Trott

Trott Apr 23, 2017

Member

I think this should be this instead:

const prefix = path.relative('.', `${common.PIPE}-net-connect-options-fd`);

Simpler, and I think it will fix the problems we're seeing on CI.

@Trott
Copy link
Member

left a comment

I think it needs a tweak in the path.relative() call. See comment.

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

ping @coreybeaumont … do you think you could address @Trott’s comment?

@coreybeaumont

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

Apologies @Trott I missed that note. I'll take of this today.

test: improved call to path.relative()
Improved the call to path.relative(). Also added comments explaining the use of a relative path.
@coreybeaumont

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@Trott added b1f370b

const prefix = path.relative(`${common.PIPE}-net-connect-options-fd`,
`${__dirname}-net-connect-options-fd`);
// Using relative path on osx if the path is greaterthan 108 chars
// an AssertionError: -48 is thrown

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 27, 2017

Member

nit: might be worthwhile adding a comment that explains what -48 means.

This comment has been minimized.

Copy link
@coreybeaumont

coreybeaumont Apr 27, 2017

Author Contributor

@jasnell That unfortunately I don't know. I believe @addaleax does?

This comment has been minimized.

Copy link
@Trott

Trott Apr 27, 2017

Member

I believe -48 is the libuv error for UV_EADDRINUSE. In any event, it's a libuv error.

I don't think this is OS X specific. It will happen on anything (or at least anything POSIX?) with a long enough path. I think you can just remove the comment (no one's going to wonder "why aren't they using an absolute path here??!!") or use a comment like this:

// Use relative path to avoid hitting 128-char length limit for socket paths in libuv.

/cc @addaleax to make sure I got that right (that it's 128 chars, that it's in libuv, etc.).

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 27, 2017

Member

@Trott 108 should be the right number on Linux and almost certainly other systems too¹², quote unix(7):

struct sockaddr_un {
    sa_family_t sun_family;               /* AF_UNIX */
    char        sun_path[108];            /* pathname */
};

It’s this bit of code in libuv that truncates the path to that size. The actual value for EADDRINUSE is OS-dependent; for example, for me on Linux it’s 98 (which libuv forwards as -98).

See also #12708, I think that’s the same kind of problem.

¹ edit: yeah, your guess of “anything POSIX” sounds right, Windows doesn’t have UNIX sockets. ;)

² more edits: https://nodejs.org/api/net.html#net_server_listen_path_backlog_callback has numbers, it’s somewhere between 92 and 108.

@Trott
Trott approved these changes Apr 27, 2017
Copy link
Member

left a comment

LGTM if CI is green.

@Trott

This comment has been minimized.

test: improved comments
Altered to comment to better reflect the use of a relative path.
addaleax added a commit to addaleax/libuv that referenced this pull request Apr 27, 2017
unix: fail with ENAMETOOLONG in `uv_pipe_*`
Fail with ENAMETOOLONG when the name of a Unix socket exceeds
`sizeof(saddr.sun_path)`. Previously the path was just truncated,
which could result in nasty bugs, and even though that behaviour
has been always been around, it’s hard to imagine a situation in
which ending up with an incorrect path is better than not creating
a socket at all.

Ref: nodejs/node#12601
Ref: nodejs/node#12708
addaleax added a commit to addaleax/libuv that referenced this pull request Apr 27, 2017
unix: fail with ENAMETOOLONG in `uv_pipe_*`
Fail with ENAMETOOLONG when the name of a Unix socket exceeds
`sizeof(saddr.sun_path)`. Previously the path was just truncated,
which could result in nasty bugs, and even though that behaviour
has been always been around, it’s hard to imagine a situation in
which ending up with an incorrect path is better than not creating
a socket at all.

Ref: nodejs/node#12601
Ref: nodejs/node#12708
Trott added a commit to Trott/io.js that referenced this pull request Apr 28, 2017
test: add relative path to accommodate limit
Found that libuv had a path character limit of 108.

Used path.relative() to set prefix with relative path.

Also added comments explaining the use of a relative path.

PR-URL: nodejs#12601
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

Landed in 0101a8f.
Thanks for the contribution! 🎉

@Trott Trott closed this Apr 28, 2017

@jasnell jasnell referenced this pull request May 11, 2017
santigimeno added a commit to libuv/libuv that referenced this pull request May 31, 2017
unix: fail with ENAMETOOLONG in `uv_pipe_*`
Fail with ENAMETOOLONG when the name of a Unix socket exceeds
`sizeof(saddr.sun_path)`. Previously the path was just truncated,
which could result in nasty bugs, and even though that behaviour
has been always been around, it’s hard to imagine a situation in
which ending up with an incorrect path is better than not creating
a socket at all.

Refs: nodejs/node#12601
Refs: nodejs/node#12708
PR-URL: #1329
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Should land with #11847 if that is backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.