cluster: correctly handle relative unix socket paths #16749

laino · 2017-11-04T16:08:18Z

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well.

I only added the "find the shortest possible path for unix sockets" to the cluster code, so that
outside of workers the net API would still map to the actual syscalls happening under the hood
cleanly.

Fixes: #16387

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)

net, cluster

mscdex · 2017-11-04T16:59:02Z

I feel like this should be done more upstream (in node core) to avoid having to potentially call path methods very often.

lpinca · 2017-11-04T16:58:01Z

lib/internal/cluster/child.js

@@ -58,11 +65,13 @@ cluster._getServer = function(obj, options, cb) {
  else
    indexes[indexesKey]++;

-  const message = util._extend({
+  const message = Object.assign({


AFAIK we are still using util._extend() instead Object.assign() for performance reasons.

lpinca · 2017-11-04T16:58:47Z

test/parallel/test-cluster-net-listen-relative-path.js

@@ -0,0 +1,45 @@
+// Copyright Joyent, Inc. and other Node contributors.


New tests don't need the copyright comment.

cjihrig · 2017-11-04T18:48:10Z

test/parallel/test-cluster-net-listen-relative-path.js

+  }));
+} else {
+  process.chdir('./unix-socket-dir');
+  net.createServer(common.mustNotCall()).listen('./cluster.sock', function() {


Can you add a common.mustCall() to the callback.

cjihrig · 2017-11-04T18:49:01Z

test/parallel/test-cluster-net-listen-relative-path.js

+      fs.existsSync('./cluster.sock'), true,
+      'cluster.sock created in CWD');
+
+    process.exit(0);


Instead of forcing the exit, can you cleanup as necessary and let the process exit naturally.

laino · 2017-11-04T20:01:18Z

Do you like your PR commits --amend'ed or with a second commit?

addaleax · 2017-11-04T20:03:15Z

@laino Whatever is easier for you :) We’re going to squash commits when landing them anyway

laino · 2017-11-15T00:17:59Z

Hey, is there anything else that needs changing for this to be merged?

joyeecheung · 2017-11-20T13:44:41Z

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

joyeecheung · 2017-11-21T21:13:23Z

@laino Related failures on all Windows builds:

not ok 79 parallel/test-cluster-net-listen-relative-path
  ---
  duration_ms: 0.606
  severity: fail
  stack: |-
    events.js:136
          throw er; // Unhandled 'error' event
          ^
    
    Error: bind EACCES ./cluster.sock
        at Object._errnoException (util.js:1010:13)
        at _exceptionWithHostPort (util.js:1031:20)
        at listenOnMasterHandle (net.js:1427:16)
        at rr (internal/cluster/child.js:120:12)
        at Worker.send (internal/cluster/child.js:87:7)
        at process.onInternalMessage (internal/cluster/utils.js:42:8)
        at process.emit (events.js:164:20)
        at emit (internal/child_process.js:790:12)
        at _combinedTickCallback (internal/process/next_tick.js:140:11)
        at process._tickCallback (internal/process/next_tick.js:179:9)
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at Worker.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-cluster-net-listen-relative-path.js:13:12)
        at Worker.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:522:15)
        at Worker.emit (events.js:159:13)
        at ChildProcess.worker.process.once (internal/cluster/master.js:185:12)
        at Object.onceWrapper (events.js:254:19)
        at ChildProcess.emit (events.js:159:13)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:209:12)
  ...

laino · 2017-11-21T21:46:11Z

@joyeecheung That's the test I added with this PR. It should probably be disabled on windows, but I'm not sure. I don't know much about named pipes on windows.

laino · 2017-11-21T21:53:53Z

I'll check tomorrow whether there's any tests for named pipes at all for windows, since I'm worried now this might break something. Also I should probably change https://github.com/nodejs/node/pull/16749/files#diff-a8eff7604cf3ad3580c1949c54176127R282 to only happen on Linux, since it doesn't make much sense on systems that don't have the arbitrary 100 byte limitation.

refack · 2017-11-21T22:09:50Z

Hello @laino, Welcome, and thank you for the contribution! 🥇

Named pipes on Windows live in their own FS namespace, and must be prefixed with \\\\.\\pipe\\. So yes the change is only relevant if (process.platform != 'win32' || !address.startsWith('\\\\.\\pipe\\')).
As for the test, I'd make a socket name 'xy'.repeat(45), so that full path will be > 92 while relative will be 92. For skipping put this just after the definition of common at the top of the file:

if (common.isWindows)
  common.skip('no 100 byte limit on pipe names on Windows');

addaleax · 2017-12-10T02:49:42Z

ping @laino ... If you have the opportunity, could you try to fix this on Windows, or if not, could you let us know?

BridgeAR · 2018-01-05T15:05:21Z

Ping @laino

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well. Fixes: nodejs#16387

laino · 2018-01-08T20:07:42Z

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

laino · 2018-01-08T20:09:00Z

Sorry, I had all but forgotten about this PR.

I made sure the test won't run on windows and that the code to deal with unix domain sockets
won't do anything on windows platforms.

joyeecheung · 2018-01-09T18:13:38Z

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

laino · 2018-01-10T07:10:38Z

https://ci.nodejs.org/job/node-test-commit-arm/13003/nodes=ubuntu1604-arm64_odroid_c2/console

This is the build that failed. Doesn't seem to be related to this?

joyeecheung · 2018-01-10T14:11:42Z

@laino That's a known OOM issue, the CI looks green to me.

joyeecheung · 2018-01-10T14:13:35Z

test/parallel/test-cluster-net-listen-relative-path.js

+
+// Ref:
+// https://github.com/nodejs/node/issues/16387
+// https://github.com/nodejs/node/pull/16749


These refs can be removed during landing. They should be in the metadata, not in the comments.

joyeecheung · 2018-01-17T00:46:31Z

CI before landing https://ci.nodejs.org/job/node-test-pull-request/12568/

joyeecheung · 2018-01-17T04:09:07Z

Landed in 4e2e7d1, thanks!

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well. PR-URL: #16749 Fixes: #16387 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

bnoordhuis · 2018-01-20T00:04:49Z

Back-porters: should be back-ported along with #18263.

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well. PR-URL: #16749 Fixes: #16387 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins · 2018-02-27T05:47:04Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well. PR-URL: #16749 Fixes: #16387 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Nov 4, 2017

lpinca reviewed Nov 4, 2017

View reviewed changes

addaleax approved these changes Nov 4, 2017

View reviewed changes

cjihrig reviewed Nov 4, 2017

View reviewed changes

laino force-pushed the dev-worker-listen branch from 4f3e58f to 9ce5f77 Compare November 4, 2017 20:09

jasnell approved these changes Nov 16, 2017

View reviewed changes

refack self-assigned this Nov 21, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

cluster: correctly handle relative unix socket paths

e9d6c4b

Relative unix sockets paths were previously interpreted relative to the master's CWD, which was inconsistent with non-cluster behavior. A test case has been added as well. Fixes: nodejs#16387

laino force-pushed the dev-worker-listen branch from 9ce5f77 to e9d6c4b Compare January 8, 2018 20:06

joyeecheung approved these changes Jan 10, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 10, 2018

joyeecheung closed this Jan 17, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added backport-requested-v6.x labels Feb 27, 2018

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: correctly handle relative unix socket paths #16749

cluster: correctly handle relative unix socket paths #16749

laino commented Nov 4, 2017

mscdex commented Nov 4, 2017 •

edited

Loading

lpinca Nov 4, 2017

lpinca Nov 4, 2017

cjihrig Nov 4, 2017

cjihrig Nov 4, 2017

laino commented Nov 4, 2017

addaleax commented Nov 4, 2017

laino commented Nov 15, 2017

joyeecheung commented Nov 20, 2017

joyeecheung commented Nov 21, 2017 •

edited

Loading

laino commented Nov 21, 2017

laino commented Nov 21, 2017

refack commented Nov 21, 2017

addaleax commented Dec 10, 2017

BridgeAR commented Jan 5, 2018

laino commented Jan 8, 2018

laino commented Jan 8, 2018 •

edited

Loading

joyeecheung commented Jan 9, 2018

laino commented Jan 10, 2018

joyeecheung commented Jan 10, 2018

joyeecheung Jan 10, 2018

joyeecheung commented Jan 17, 2018

joyeecheung commented Jan 17, 2018

bnoordhuis commented Jan 20, 2018

MylesBorins commented Feb 27, 2018

		@@ -0,0 +1,45 @@
		// Copyright Joyent, Inc. and other Node contributors.

cluster: correctly handle relative unix socket paths #16749

cluster: correctly handle relative unix socket paths #16749

Conversation

laino commented Nov 4, 2017

Checklist

Affected core subsystem(s)

mscdex commented Nov 4, 2017 • edited Loading

lpinca Nov 4, 2017

Choose a reason for hiding this comment

lpinca Nov 4, 2017

Choose a reason for hiding this comment

cjihrig Nov 4, 2017

Choose a reason for hiding this comment

cjihrig Nov 4, 2017

Choose a reason for hiding this comment

laino commented Nov 4, 2017

addaleax commented Nov 4, 2017

laino commented Nov 15, 2017

joyeecheung commented Nov 20, 2017

joyeecheung commented Nov 21, 2017 • edited Loading

laino commented Nov 21, 2017

laino commented Nov 21, 2017

refack commented Nov 21, 2017

addaleax commented Dec 10, 2017

BridgeAR commented Jan 5, 2018

laino commented Jan 8, 2018

laino commented Jan 8, 2018 • edited Loading

joyeecheung commented Jan 9, 2018

laino commented Jan 10, 2018

joyeecheung commented Jan 10, 2018

joyeecheung Jan 10, 2018

Choose a reason for hiding this comment

joyeecheung commented Jan 17, 2018

joyeecheung commented Jan 17, 2018

bnoordhuis commented Jan 20, 2018

MylesBorins commented Feb 27, 2018

mscdex commented Nov 4, 2017 •

edited

Loading

joyeecheung commented Nov 21, 2017 •

edited

Loading

laino commented Jan 8, 2018 •

edited

Loading