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

worker: support relative paths #21407

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@itaysabato
Contributor

itaysabato commented Jun 19, 2018

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

This comment has been minimized.

Member

addaleax commented Jun 19, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 19, 2018

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

This comment has been minimized.

Member

benjamingr commented Jun 19, 2018

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

This comment has been minimized.

Member

benjamingr commented Jun 19, 2018

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

@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jun 19, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 19, 2018

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

This comment has been minimized.

Member

benjamingr commented Jun 20, 2018

@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

This comment has been minimized.

Member

benjamingr commented Jun 20, 2018

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

This comment has been minimized.

Member

addaleax commented Jun 20, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 20, 2018

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

targos approved these changes Jun 20, 2018

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

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 20, 2018

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

This comment has been minimized.

Member

benjamingr commented Jun 20, 2018

allowing relative paths is going to conflict with URL strings

Can you explain this?

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 20, 2018

@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

This comment has been minimized.

Member

benjamingr commented Jun 20, 2018

@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

This comment has been minimized.

Member

targos commented Jun 20, 2018

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

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 20, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 24, 2018

@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

This comment has been minimized.

Member

benjamingr commented Jun 24, 2018

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

This comment has been minimized.

Member

addaleax commented Jun 24, 2018

@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

This comment has been minimized.

Contributor

guybedford commented Jun 24, 2018

@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

This comment has been minimized.

Member

addaleax commented Jun 24, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 24, 2018

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.

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

This comment has been minimized.

@guybedford

guybedford Jun 24, 2018

Contributor

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

This comment has been minimized.

@itaysabato

itaysabato Jun 24, 2018

Contributor

On it.

itaysabato added a commit to itaysabato/node that referenced this pull request Jun 24, 2018

worker: support only relative paths starting with `./` or `../`
This commit handles relative paths not starting with `./` or `../` by throwing an error.

PR-URL: nodejs#21407
@benjamingr

This comment has been minimized.

Member

benjamingr commented Jun 24, 2018

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

@benjamingr

This comment has been minimized.

if (!path.isAbsolute(filename) &&
!filename.startsWith('./') &&
!filename.startsWith('../')) {

This comment has been minimized.

@addaleax

addaleax Jun 24, 2018

Member

I think indentation is off here?

This comment has been minimized.

@itaysabato

itaysabato Jun 24, 2018

Contributor

Lint didn't seem to have a problem with it, so not sure what should be different...

This comment has been minimized.

@addaleax

addaleax Jun 24, 2018

Member

We usually line up the individual clauses vertically (and omit a blank link at the beginning of a block) :)

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

This comment has been minimized.

@jasnell

jasnell Jun 24, 2018

Member

why the nextTick here? Seems a bit pointless.

This comment has been minimized.

@itaysabato

itaysabato Jun 24, 2018

Contributor

Actually it was simply copied from test-worker.js

This comment has been minimized.

@addaleax

addaleax Jun 24, 2018

Member

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

This comment has been minimized.

@itaysabato

itaysabato Jun 24, 2018

Contributor

Got it, thanks.

@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jun 24, 2018

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

@benjamingr

This comment has been minimized.

Member

benjamingr commented Jun 24, 2018

CI failures seem unrelated

itaysabato added a commit to itaysabato/node that referenced this pull request Jun 24, 2018

worker: support relative paths
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 <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@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

This comment has been minimized.

Contributor

itaysabato commented Jun 25, 2018

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

@apapirovski

This comment has been minimized.

Member

apapirovski commented Jun 25, 2018

@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

This comment has been minimized.

Contributor

itaysabato commented Jun 25, 2018

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

@targos

This comment has been minimized.

Member

targos commented Jun 25, 2018

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

This comment has been minimized.

Contributor

itaysabato commented Jun 26, 2018

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

***fixing...

worker: support relative paths
This commit adds support for relative paths in Worker.
Paths are relative to the current working directory.

PR-URL: #21407
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jun 26, 2018

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

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 26, 2018

@itaysabato

This comment has been minimized.

Contributor

itaysabato commented Jun 26, 2018

thanks :)

benjamingr added a commit that referenced this pull request Jun 27, 2018

worker: support relative paths
This commit adds support for relative paths in Worker.
Paths are relative to the current working directory.

PR-URL: #21407
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@benjamingr

This comment has been minimized.

Member

benjamingr commented Jun 27, 2018

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.

@benjamingr benjamingr closed this Jun 27, 2018

@itaysabato itaysabato deleted the itaysabato:workers-relative-path branch Jun 27, 2018

targos added a commit that referenced this pull request Jun 28, 2018

worker: support relative paths
This commit adds support for relative paths in Worker.
Paths are relative to the current working directory.

PR-URL: #21407
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
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)

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
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

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
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

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment