worker: support relative paths #21407

itaysabato · 2018-06-19T17:32:08Z

This commit adds support to relative paths in worker.

Hi, this is my first pull request in Node.js.

@benjamingr helped me find this issue.

CC @nodejs/workers @addaleax

Checklist

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

benjamingr

Thanks for opening a pull request 🎉 .

Actual changes LGTM and a few people I talked to have run into this so +1 on the actual change too.

addaleax · 2018-06-19T18:07:37Z

The catch here is that this may lead to surprising results if the program is run from a different working directory than what is expected. If we do support relative paths, I’d much prefer it if we could make it relative to the path of the caller script…

itaysabato · 2018-06-19T18:28:21Z

Hi @addaleax
Thanks for the quick feedback :)

I find that it would be very counter-intuitive for the path to be relative to the caller script. For instance, if I call fs.readFileSync('hello.js') the file path hello.js will be resolved against the working directory and not the caller script. Why should workers be any different?

benjamingr · 2018-06-19T19:01:02Z

I would expect worker to behave like require and not like fs.readFileSync (I'm writing this before checking whether or not require behaves like fs.readFileSync and this PR or differently)

benjamingr · 2018-06-19T19:17:54Z

I think changing it would require just resolving with path.join on process.cwd() and __filename and then .resolve like this PR?

itaysabato · 2018-06-19T19:18:44Z

Fair point @benjamingr.

I tend to agree, though that would mean hello.js should be looked up in node_modules etc. and ./hello.js would be resolved relative to the caller script folder (even though they are equivalent when calling e.g. fs.readFile).

Is that what you're going for?

~~Also not actually sure how require('/hello.js') is looked up, is it compatible with the way the worker resolves it today?~~

itaysabato · 2018-06-19T23:02:05Z

Come to think of it, I would say new Worker('hello.js') is closer to child_process.fork('hello.js') (which resolves to the working directory and does not lookup node_modules) than to require('hello.js'). It's not like we're "importing" a module using another thread - the code is much more isolated than modules we load with require.

Also, I really don't know how to get the caller file path without hacking into the stack trace or something like that. It is a bit weird to expect a constructor to "know" which file his caller comes from. For an import statement (and require for the time being) it make sense as part of static analysis.

benjamingr · 2018-06-20T09:26:57Z

@itaysabato so the current (non process.cwd) behaviour is similar to how child_process.fork('hello.js') behaves? I guess that also makes sense.

benjamingr · 2018-06-20T09:31:43Z

I tend to agree, though that would mean hello.js should be looked up in node_modules etc.

I was explicitly referring to the CWD behaviour - although you can require.resolve the file. I don't feel strongly about doing this. I'm more concerned about the majority use case of the CWD not changing although I'll defer to Anna if she thinks it's important.

addaleax · 2018-06-20T09:36:50Z

about the majority use case of the CWD not changing

The thing is, this being the majority use case makes it easy to write code under that assumption…

As you noticed, I’m not putting a red X on this or anything. I’m saying that this was a choice I made and that, if other people think it makes sense, I’m okay with it being aligned with somebody else’s expectations rather than mine. :)

itaysabato · 2018-06-20T10:57:26Z

so the current (non process.cwd) behaviour is similar to how child_process.fork('hello.js') behaves?

I'm not sure why you call it "non process.cwd" because the current code calls path.resolve which does so relative to the current working directory, or am I missing something? In any case, from what I gather from the docs and local tests I've done so far this is how child_process.fork behaves by default.

I didn't know require.resolve - looks very useful actually! But if I call it from the Worker constructor it won't be of any use. The fact that require.resolve exists is actually an argument for leaving the PR as is, since you can simply call new Worker(require.resolve('hello.js')) to get require-like loading.

targos

LGTM. Relative to cwd is what I expect. It is also closer to how Web Workers do it imo

addaleax · 2018-06-20T13:53:09Z

One possible concern that I realized today: If we work towards full web compat, then allowing relative paths is going to conflict with URL strings and I think we’d need to either undo this or control it through the options object here. :/

Maybe we could use new Worker(path, { relative: true }) or so?

benjamingr · 2018-06-20T13:57:23Z

allowing relative paths is going to conflict with URL strings

Can you explain this?

addaleax · 2018-06-20T14:01:44Z

@benjamingr One pain point we’ve had with fs is that we cannot allow file:// URL strings as input to the fs functions, because file:// URLs also happen to be perfectly valid relative paths – I’d like to avoid that ambiguitiy here

benjamingr · 2018-06-20T14:07:02Z

@addaleax I'm -1 on accepting URL strings to worker - I'm +1 on accepting URL objects at some point in the future though though some people (namely @issacs) have objected to this due to the burden on third-party authors and security implications.

Then again unlike file-reading I really don't see dynamic worker URLs as something too common.

targos · 2018-06-20T14:07:55Z

What about throwing for now if a valid URL is passed?

addaleax · 2018-06-20T14:13:37Z

I'm -1 on accepting URL strings to worker

That seems like it would be at odds with ever making Worker fully Web-compatible? /cc @devsnek

itaysabato · 2018-06-24T06:57:46Z

@addaleax what if instead of opting in for relative paths users would opt in for urls?

E.g. new Worker(url, { url: true })

Does that make sense?

benjamingr · 2018-06-24T07:04:33Z

Going to do one last ping to @devsnek to make sure they are OK with the behavior and land it tomorrow if there are no objections.

addaleax · 2018-06-24T11:16:35Z

@itaysabato That makes sense if we don’t expect URL usage by default / don’t care about the fact that WebWorkers use URLs … my gut feeling is still that we would rather want to put relative paths behind an option, but I can see valid points for either side here :)

guybedford · 2018-06-24T11:24:21Z

@addaleax if you really think supporting plain relative paths might conflict with URL strings then I agree this is a serious concern indeed. I thought that was only the case for fs because of how entrenched the workflows are, and would have assumed that such a brutal edge case wouldn't need to apply here for backwards compatibility concerns in future.

But if it is a concern, then this could be restricted to only an absolute or relative path (starting with ./ or .\\) only here (./x.js but not x.js), so that we don't conflict with the URL protocol cases at all.

addaleax · 2018-06-24T11:35:44Z

file:// URL strings are, as far as I know, always also valid relative paths, so yes they are going to conflict. With fs, the added concern is that existing path validators wouldn’t catch that URL strings would imply absolute paths, but the ambiguity is always there for Workers too…

itaysabato · 2018-06-24T11:46:28Z

Another way to postpone the URL support decision is to only support relative paths starting with ./ or ../ and throw otherwise.

guybedford

I do think it's important to ensure the best outcome for URL compatibility here.

guybedford · 2018-06-24T13:58:40Z

lib/internal/worker.js

-      if (!path.isAbsolute(filename)) {
-        throw new ERR_WORKER_NEED_ABSOLUTE_PATH(filename);
-      }
+      filename = path.resolve(filename);


I would prefer if we can add the filter here to only apply path.resolve for absolute, ./ and ../ paths.

benjamingr · 2018-06-24T15:53:06Z

@addaleax @targos can you please confirm you are OK with the current form of the PR?

jasnell · 2018-06-24T22:18:29Z

test/parallel/test-worker-relative-path-double-dot.js

+  }));
+} else {
+  setImmediate(() => {
+    process.nextTick(() => {


why the nextTick here? Seems a bit pointless.

Actually it was simply copied from test-worker.js

For context, inside that test the purpose is to make sure that both libuv bindings + process.nextTick() work – we don’t need that here, and I probably should have added a comment to the other test about it ;)

Got it, thanks.

itaysabato · 2018-06-24T23:08:10Z

I can't tell from the build logs what's wrong.
I'm pretty sure it was passing before I re-based.

benjamingr · 2018-06-24T23:19:58Z

CI failures seem unrelated

apapirovski

As far as I can tell this isn't system aware hence the CI failures. Feel free to dismiss my objection if I'm incorrect.

The explicit test for './' and '../' seems to point in that direction though...

itaysabato · 2018-06-25T07:05:06Z

@apapirovski as far as I can see the build is passing now.
Do you think there is still a problem?

apapirovski · 2018-06-25T07:08:17Z

@itaysabato The last CI I can see is red and certainly had related failures.

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

itaysabato · 2018-06-25T07:16:57Z

Ah I see, so the Travis build doesn't cover everything Jenkins covers?
Does Jenkins also run automatically?

targos · 2018-06-25T07:44:41Z

Ah I see, so the Travis build doesn't cover everything Jenkins covers?

Travis only covers linting and testing on Linux.

Does Jenkins also run automatically?

No. A collaborator has to manually trigger a Jenkins run.

itaysabato · 2018-06-26T14:15:55Z

Finally managed to build on a windows vm.
@apapirovski is right - need to use path.sep instead of /.

***fixing...

This commit adds support for relative paths in Worker. Paths are relative to the current working directory. PR-URL: nodejs#21407 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

itaysabato · 2018-06-26T16:26:09Z

Should be fixed now.
Can someone please run CI again?

addaleax · 2018-06-26T17:21:23Z

Sure :)

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

itaysabato · 2018-06-26T19:59:41Z

thanks :)

This commit adds support for relative paths in Worker. Paths are relative to the current working directory. PR-URL: #21407 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

benjamingr · 2018-06-27T07:41:00Z

CI is now green, good catch Anatoli and thanks for fixing Itay!

Landed in 8d33bbf 🎉 I hope this is your first PR in many!

Random nit - PR-URL should be a URL :) I amended that for you.

This commit adds support for relative paths in Worker. Paths are relative to the current working directory. PR-URL: #21407 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Notable changes: * build: * Node.js should now be about 60% faster to startup than the previous version, thanks to the use V8's code cache feature for core modules. [#21405](#21405) * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629

Notable changes: * dns: * An experimental promisified version of the dns module is now available. Give it a try with `require('dns').promises`. [#21264](#21264) * fs: * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498) * lib: * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript specification ([reference](tc39/ecma262#1220)). Since Node.js now has experimental support for worker threads, we are being proactive and added a `notify` alias, while emitting a warning if `wake` is used. [#21413](#21413) [#21518](#21518) * n-api: * Add API for asynchronous functions. [#17887](#17887) * util: * `util.inspect` is now able to return a result instead of throwing when the maximum call stack size is exceeded during inspection. [#20725](#20725) * vm: * Add `script.createCachedData()`. This API replaces the `produceCachedData` option of the `Script` constructor that is now deprecated. [#20300](#20300) * worker: * Support for relative paths has been added to the `Worker` constructor. Paths are interpreted relative to the current working directory. [#21407](#21407) PR-URL: #21629

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. worker Issues and PRs related to Worker support. labels Jun 19, 2018

benjamingr approved these changes Jun 19, 2018

View reviewed changes

targos approved these changes Jun 20, 2018

View reviewed changes

guybedford requested changes Jun 24, 2018

View reviewed changes

guybedford approved these changes Jun 24, 2018

View reviewed changes

jasnell reviewed Jun 24, 2018

View reviewed changes

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

itaysabato force-pushed the workers-relative-path branch from 9348fe0 to 8defed3 Compare June 24, 2018 23:28

apapirovski requested changes Jun 25, 2018

View reviewed changes

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

itaysabato force-pushed the workers-relative-path branch from ca338a3 to c31ae12 Compare June 26, 2018 16:16

benjamingr closed this Jun 27, 2018

itaysabato deleted the workers-relative-path branch June 27, 2018 08:08

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

addaleax mentioned this pull request Jul 6, 2018

Worker relative path resolution fails with nested folders #21686

Closed

worker: support relative paths #21407

worker: support relative paths #21407

Conversation

itaysabato commented Jun 19, 2018 • edited by benjamingr Loading

Checklist

benjamingr left a comment

Choose a reason for hiding this comment

addaleax commented Jun 19, 2018

itaysabato commented Jun 19, 2018

benjamingr commented Jun 19, 2018

benjamingr commented Jun 19, 2018

itaysabato commented Jun 19, 2018 • edited Loading

itaysabato commented Jun 19, 2018

benjamingr commented Jun 20, 2018

benjamingr commented Jun 20, 2018

addaleax commented Jun 20, 2018

itaysabato commented Jun 20, 2018

targos left a comment

Choose a reason for hiding this comment

addaleax commented Jun 20, 2018

benjamingr commented Jun 20, 2018

addaleax commented Jun 20, 2018

benjamingr commented Jun 20, 2018

targos commented Jun 20, 2018

addaleax commented Jun 20, 2018

itaysabato commented Jun 24, 2018

benjamingr commented Jun 24, 2018

addaleax commented Jun 24, 2018

guybedford commented Jun 24, 2018

addaleax commented Jun 24, 2018

itaysabato commented Jun 24, 2018

guybedford left a comment

Choose a reason for hiding this comment

guybedford Jun 24, 2018

Choose a reason for hiding this comment

itaysabato Jun 24, 2018

Choose a reason for hiding this comment

benjamingr commented Jun 24, 2018

jasnell Jun 24, 2018

Choose a reason for hiding this comment

itaysabato Jun 24, 2018

Choose a reason for hiding this comment

addaleax Jun 24, 2018

Choose a reason for hiding this comment

itaysabato Jun 24, 2018

Choose a reason for hiding this comment

itaysabato commented Jun 24, 2018

benjamingr commented Jun 24, 2018

apapirovski left a comment

Choose a reason for hiding this comment

itaysabato commented Jun 25, 2018

apapirovski commented Jun 25, 2018

itaysabato commented Jun 25, 2018

targos commented Jun 25, 2018

itaysabato commented Jun 26, 2018

itaysabato commented Jun 26, 2018

addaleax commented Jun 26, 2018

itaysabato commented Jun 26, 2018

benjamingr commented Jun 27, 2018 • edited Loading

itaysabato commented Jun 19, 2018 •

edited by benjamingr

Loading

itaysabato commented Jun 19, 2018 •

edited

Loading

benjamingr commented Jun 27, 2018 •

edited

Loading