test: Added relative path to accommodate limit by b6t · Pull Request #12601 · nodejs/node

b6t · 2017-04-22T20:28:48Z

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)

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

Trott

LGTM if CI is green. Thanks for fixing this!

Trott · 2017-04-22T20:33:24Z

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

mscdex · 2017-04-22T21:19:51Z

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)).

Trott · 2017-04-23T02:26:52Z

test/parallel/test-net-connect-options-fd.js

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


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

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

addaleax · 2017-04-27T15:33:31Z

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

b6t · 2017-04-27T16:13:55Z

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

Improved the call to path.relative(). Also added comments explaining the use of a relative path.

b6t · 2017-04-27T18:42:40Z

@Trott added b1f370b

jasnell · 2017-04-27T18:45:05Z

test/parallel/test-net-connect-options-fd.js

-  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


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

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

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.).

@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

LGTM if CI is green.

Trott · 2017-04-27T22:17:46Z

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

Altered to comment to better reflect the use of a relative path.

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

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 · 2017-04-28T21:40:38Z

Landed in 0101a8f.
Thanks for the contribution! 🎉

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 · 2017-06-18T22:43:32Z

Should land with #11847 if that is backported.

test: Added relative path to accommodate limit

f7a42ed

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

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 22, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Apr 22, 2017

addaleax approved these changes Apr 22, 2017

View reviewed changes

Trott approved these changes Apr 22, 2017

View reviewed changes

cjihrig approved these changes Apr 22, 2017

View reviewed changes

mscdex added the net Issues and PRs related to the net subsystem. label Apr 22, 2017

Trott reviewed Apr 23, 2017

View reviewed changes

Trott requested changes Apr 23, 2017

View reviewed changes

test: improved call to path.relative()

b1f370b

Improved the call to path.relative(). Also added comments explaining the use of a relative path.

jasnell reviewed Apr 27, 2017

View reviewed changes

Trott approved these changes Apr 27, 2017

View reviewed changes

test: improved comments

d97a6d8

Altered to comment to better reflect the use of a relative path.

jasnell approved these changes Apr 27, 2017

View reviewed changes

addaleax mentioned this pull request Apr 27, 2017

unix: fail with ENAMETOOLONG in uv_pipe_* libuv/libuv#1329

Closed

gibfahn approved these changes Apr 28, 2017

View reviewed changes

Trott closed this Apr 28, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

This was referenced Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

test: test various calling patterns of net.connect() related functions #11847

Closed

gibfahn added the backport-requested-v6.x label Jun 18, 2017

Uh oh!

Conversation

b6t commented Apr 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 22, 2017

Uh oh!

mscdex commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 27, 2017

Uh oh!

b6t commented Apr 27, 2017

Uh oh!

b6t commented Apr 27, 2017

Uh oh!

jasnell Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

b6t Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 27, 2017

Uh oh!

Trott commented Apr 28, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mscdex commented Apr 22, 2017 •

edited

Loading

Trott Apr 27, 2017 •

edited

Loading

addaleax Apr 27, 2017 •

edited

Loading