worker: initial implementation #40

addaleax · 2017-09-02T04:41:05Z

edit: moved to #58

(status: currently ready for review, by anyone, including you: #40 (comment))

'use strict';
const { Worker } = require('worker');

if (process.isMainThread) {
  module.exports = async function parseJSAsync(script) {
    return new Promise((resolve, reject) => {
      const worker = new Worker(__filename, {
        workerData: script
      });
      worker.on('message', resolve);
      worker.on('error', reject);
      worker.on('exit', (code) => {
        if (code !== 0)
          reject(new Error(`Worker stopped with exit code ${code}`));
      });
    });
  };
} else {
  const { parse } = require('some-js-parsing-library');
  const script = process.workerData;
  process.postMessage(parse(script));
}

(If that gives the wrong impression: No, this does not conform the browser WebWorker API, but that should be rather easily implementable on top of this, and I’m okay with that.)

Fixes: #31

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

TODO

(@petkaantonov please feel free to indicate whether attributing you for code that comes from your original PR is not enough/too much/just right :) )

benjamingr · 2017-09-02T08:51:03Z

lib/domain.js

@@ -21,6 +21,10 @@

 'use strict';

+if (process.isWorkerThread) {
+  throw new Error('domains are not available inside of worker threads');


That's a pretty big decision to make. Personally I'm +1 on it but it means that any code that currently runs in child processes won't be able to run on workers.

Petka's original idea was to convert cluster to use workers.

it means that any code that currently runs in child processes won't be able to run on workers.

You mean “some code”, right? As in, child processes and workers are incompatible? I’m okay with that as well.

Petka's original idea was to convert cluster to use workers.

I would assume by now too much code relies on that not being the case anyway.

You mean “some code”, right? As in, child processes and workers are incompatible? I’m okay with that as well.

That sentence was missing "and uses domains" :)

I think that people currently using child processes to offload work are the most obvious potential consumers of workers and this might make their life harder.

I'm definitely OK with starting this way in any case.

I would assume by now too much code relies on that not being the case anyway.

I'm not sure why?

I would assume by now too much code relies on that not being the case anyway.

I'm not sure why?

For one, the current implementation in this PR doesn’t provide stdio to Workers, but I would be pretty sure a lot of cluster-using code wants that to be there ;)

Come to think of it - can't stdio just be done through a MessagePort?

@benjamingr Yes … but that could also be done by the user, right? stdio in Node/Ayo is complicated, and I’m a bit scared of what happens when multiple handles to the same resource exist (like, some could be set to blocking, data would end up being intertwined, there would be different libuv buffers, and for stdin there can’t really be any consistent behaviour anyway.

I’ve made console.* work by just using a fully synchronous stream that operates directly on the file descriptors, which might not be super-efficient but I think it’s reasonable here.

I think that's reasonable and we can always add it later.

benjamingr · 2017-09-02T08:53:14Z

lib/internal/worker.js

+    debug(`[${process.threadId}] created Worker with ID ${this.threadId}`);
+
+    // Actually start the new thread now that everything is in place.
+    this[kHandle].startThread();


I'm not sure if we shouldn't provide .start() as an external method so creating and starting the work are different, but I don't feel strongly about it since the user can always just keep a function returning a thread.

Yeah, it’s an arbitrary decision in my eyes, too. Right now, all communication with the Worker is asynchronous anyway, so I couldn’t find any advantage in adding a separate method, but if there is some, fine by me. :)

benjamingr · 2017-09-02T09:05:24Z

lib/internal/worker.js

+
+const assert = require('assert');
+const errors = require('internal/errors');
+const { Serializer, Deserializer } = require('v8');


We need a way to pass binary data fast between isolates - given we can't pass structured data (right?) I think that we need a way to pass buffers and other node builtins fast if we can.

The custom Serializer does support Node.js Buffers. Are you saying the performance of going through the serializer might be suboptimal?

I'm saying that passing binary data efficiently between workers needs to be fast, it would be amazing to be able to pass objects faster than structured-cloning them but I don't see that happening any time soon.

One of the most commonly misunderstood problems people want to solve with workers is "offload parsing a lot of JSON" where the overhead is creating all the objects and not reading the actual JSON and passing it to a worker won't help (since now 2 copies are made per worker because it's deserializaed from JSON to object, then between workers, then to the user).

I don't think we can fix that though.

@benjamingr Maybe to clarify: Are you saying the serialization (that corresponds to structured cloning, and afaik is what Chrome implements structured cloning on top of) needs to be fast, or that passing in a Buffer on the one side and getting out another Buffer on the other side needs to be fast?

Anyway:

I think we might be able to get rid of one (?) of the copy operations along the way when a Buffer is passed in by adding a fast case for “this is just an ArrayBufferView”

I would also like to add SharedArrayBuffer/transferable support, now or in the future

I don't think we can fix that though.

Yeah.

that passing in a Buffer on the one side and getting out another Buffer on the other side needs to be fast?

That, I think that passing a buffer should be fast (and optimally a binary stream). I have no hopes for anything that requires actually copying JS objects from one heap to another.

We won’t be able to get around one of the copies, because any Buffer that’s being passed in could be modified in the sending thread afterwards, but I think keeping it at a single copy should work.

@benjamingr Yeah. A FAQ/explainer for questions like these should be good, though that might sound too much like a Node.js EP... sigh

@addaleax I'm fine with modifying a sent buffer throwing or providing a different type.

I'm not too concerned with the API but I definitely think workers should have the capability to pass memory without copies somehow.

For example, if I'm using workers to transcode video and I'm doing it at 20Mbps in 4 workers - that means my process is copying 4.8 gigabits of memory every minute which is a lot of overhead.

I'm fine with modifying a sent buffer throwing or providing a different type.

Oh, right. I think we can Neuter() array buffers like that (or even have to do that when using them as transferrables?).

I'm not too concerned with the API but I definitely think workers should have the capability to pass memory without copies somehow.

I think that would be doable with SharedArrayBuffers in any case

(to be clear, leaving out support for SABs/transferrables is not a design decision of this PR, it’s a TODO of it :D)

TimothyGu · 2017-09-02T09:38:11Z

Awesome work!

I do have some questions about this implementation. It doesn't have to be as detailed (or contentious) as a Node.js EP, but I think these questions are likely to come up in the future. Please understand that, by asking these questions, I'm also genuinely interested in the answers and not trying to pass any judgements on the decisions made by this implementation.

ESM support. With ESM being an active WIP, and something that is spec'd for Web Worker, does this design for Worker allow future extension to ESM?
How much more work does implementing the Web Worker API on top of this API necessitate?
Blackboxing globals. A use case for Workers identified earlier is for execution of untrusted code. This impl already provided memory constraints, but maybe an option should be provided to not grant the Worker any special access to the system except to, well, work? (i.e. no require, no module, no process, only message posting, which i guess does sound like Web Workers)
Resource limits. As mentioned earlier, memory constraints are a thing in this impl. Is it possible to expose more system limits (e.g. ulimit) in this API in the future?
Synchronous IO. I remember @domenic mentioned synchronous IO as a potential use case for Workers, and sync XHR is indeed a thing in Web Workers. Is it possible to extend synchronocity to more core modules with something like this API in place?
SharedArrayBuffer. I'd like to confirm that this can be implemented, as it's one of the more exciting things in ES recently. Right?
Collaboration with Node.js. I do understand the rationale for posting the PR here, and I fully agree with them. But the current fact is that a lot of people capable of reviewing this PR properly have not yet hopped on Ayo.js. I'd like this PR to draw more people into Ayo.js, but IMO we should be thinking about meeting them in the middle.

EDIT: Added two more questions.

benjamingr · 2017-09-02T09:50:34Z

Very nice questions, I looked at the code and Anna will probably have more to add on top of it - but doing the best of my ability to answer:

ESM support. With ESM being an active WIP, and something that is spec'd for Web Worker, does this design for Worker allow future extension to ESM?

I don't see why not. This design (if I understand correctly) isn't interested with module loading semantics.

How much more work does implementing the Web Worker API on top of this API necessitate?

Not a lot at all - I'm not sure that's a goal though.

Blackboxing globals. A use case for Workers identified earlier is for execution of untrusted code.

To be honest that's something that I think should be solved by using the vm module inside a worker rather than making a worker do two things. I think if web had frozen realms (https://github.com/tc39/proposal-realms) when WebWorkers landed that's what they would have done too.

Synchronous IO. I remember @domenic mentioned synchronous IO as a potential use case for Workers, and sync XHR is indeed a thing in Web Workers.

I think CPU intensive work on in-process memory is a much more important goal for web workers. I think as a project Ayo (or Node.js) shouldn't provide any more tools for synchronous I/O since libuv already thread-pools blocking I/O operations like DNS lookups and they look "as asynchronous" to the user.

I'll let Domenic answer what he meant though :) (note he has asked to not be pinged in Node.js repos, I'm not sure how/if that holds here).

addaleax · 2017-09-02T09:59:24Z

I'm also genuinely interested in the answers and not trying to pass any judgements on the decisions made by this implementation.

You’re not coming across any other way :) Also, it’s fair to question my design decisions, they are somewhat ad-hoc and I wouldn’t be surprised if this PR did a shift in some other direction before it lands.

And @benjamingr’s answers are good answers :)

Resource limits. As mentioned earlier, memory constraints are a thing in this impl. Is it possible to expose more system limits (e.g. ulimit) in this API in the future?

I think ulimit does always work on a per-process level, so I would go with “probably not”?

TimothyGu · 2017-09-02T10:04:08Z

I think ulimit does always work on a per-process level, so I would go with “probably not”?

Hmm, right.

I was also more wondering more generally about per-thread resource limits, so not necessarily those exposed through ulimit. I.e. more in the sense of "what else can we do with Workers?"

addaleax · 2017-09-02T10:06:17Z

I was also more wondering more generally about per-thread resource limits, so not necessarily those exposed through ulimit. I.e. more in the sense of "what else can we do with Workers?"

If you’re thinking about limiting a Worker’s access to APIs, I’d go with the combination of a Worker + a new VM context that Benjamin mentioned, and let userland handle that case from there.

Otherwise, I think this really depends on what kind of limits you’re having in mind?

addaleax · 2017-09-03T14:41:34Z

I’ve added a TODO list in the PR description, if anybody has interest in jumping in on something specific, please let me know (here, or if that doesn’t work, ping me in the discord channel/twitter/wherever)

addaleax · 2017-09-03T19:38:08Z

@TimothyGu for your more recent questions:

SharedArrayBuffer. I'd like to confirm that this can be implemented, as it's one of the more exciting things in ES recently. Right?

Yes. V8 already has an implementation of proper message passing code that handles transferring ABs and sharing SABs through workers in d8, you can take a look at that if you like

Collaboration with Node.js. I do understand the rationale for posting the PR here, and I fully agree with them. But the current fact is that a lot of people capable of reviewing this PR properly have not yet hopped on Ayo.js. I'd like this PR to draw more people into Ayo.js, but IMO we should be thinking about meeting them in the middle.

Just to be clear, my personal rationale is more that I’m tired and frustrated with Node’s review process. That this would give Ayo a technical edge over Node is really really nice, but not the main issue for me.

I would hope that, if done right, this could also be a good chance to give people who have not felt welcome around Node.js to enter the scene. I would be more than happy to answer questions newcomers have about this PR, whether very abstract or very concretely referring to pieces of code.

If that fails, yes, we can go back to looking for Node people.

(Also, to clarify: Do you consider yourself a person “capable of reviewing this PR”? I personally would trust your review on this).

TimothyGu · 2017-09-04T02:30:54Z

Just to be clear, my personal rationale is more that I’m tired and frustrated with Node’s review process. That this would give Ayo a technical edge over Node is really really nice, but not the main issue for me.

I would hope that, if done right, this could also be a good chance to give people who have not felt welcome around Node.js to enter the scene. I would be more than happy to answer questions newcomers have about this PR, whether very abstract or very concretely referring to pieces of code.

If that fails, yes, we can go back to looking for Node people.

Thank you for this rationale for posting the PR here. It makes total sense, though I agree we'll have to wait and see regarding the reviews we will get.

(Also, to clarify: Do you consider yourself a person “capable of reviewing this PR”? I personally would trust your review on this).

To a certain extent, yes, I do. On the other hand, I would not feel comfortable signing off on this if this was to go in w/o other more capable pairs of eyes over it :)

addaleax · 2017-09-05T22:39:17Z

Fwiw, I just updated the worker module to include mostly-standard MessageChannel support, so if anybody wants to write tests for that, please write your hand :)

(There are no docs for that yet, but the API matches the MDN descriptions except that events are emitted via a Node.js-style message event instead of .onmessage)

addaleax · 2017-09-13T18:56:57Z

Okay, resolved conflicts + got CI back to passing again after the upstream update.

Qard · 2017-09-14T00:19:23Z

Just playing with it a bit right now. One thing that might trip up new users a bit is that a Buffer becomes a Uint8Array in the transfer. One might naively try to pass a buffer representing text, like file contents, into a worker and expect to call toString() on it at the other end, but that'll produce the array-style comma-separated list of numbers rather than the string they might be expecting.

Not sure if there's a way to automatically restore the buffer type on the other end or if it should just be mentioned in the docs that this happens.

Qard · 2017-09-14T00:58:11Z

Just got AssertionError [ERR_ASSERTION]: An invalid error message key was used: ERR_WORKER_OFFLINE. while playing with an http server in a worker.

https://gist.github.com/Qard/b0928fb4e92f53d4789702cd8ed2641a

addaleax · 2017-09-14T13:52:48Z

One thing that might trip up new users a bit is that a Buffer becomes a Uint8Array in the transfer.

Yes – that is tricky. On the one hand, if we want to follow the Web’s structured cloning algorithm, we should stick to it, and we’re supporting serialization between vm contexts here which just don’t always have Buffer. Plus, most of Node’s API doesn’t make a difference between Buffers and Uint8Arrays anymore anyway.

On the other hand, I get that people might expect Buffer → Buffer serialization, and we should probably warn against sending buffers created with Buffer.allocUnsafe() or Buffer.allocUnsafeSlow() that might contain privileged information …

Just got AssertionError [ERR_ASSERTION]: An invalid error message key was used: ERR_WORKER_OFFLINE. while playing with an http server in a worker.

Yeah… my bad. :) I’ll align the behaviour with what WebWorkers do (i.e. just ignore posted messages/terminate requests for already-terminated workers)

Taken from petkaantonov/io.js@ea143f7 and modified to fit current linter rules and coding style.

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

This should help a lot with actual sandboxing of JS code.

addaleax · 2017-09-15T18:41:21Z

Moving to #58 after we renamed branches

addaleax added the worker label Sep 2, 2017

benjamingr reviewed Sep 2, 2017

View reviewed changes

addaleax force-pushed the workers-impl branch from a2c7227 to 5a29c75 Compare September 2, 2017 19:01

addaleax force-pushed the workers-impl branch 14 times, most recently from 155c764 to ce036b7 Compare September 5, 2017 22:38

addaleax force-pushed the workers-impl branch 2 times, most recently from 06f44c0 to 69b4c36 Compare September 6, 2017 01:12

addaleax force-pushed the workers-impl branch 2 times, most recently from debb7ee to fb81652 Compare September 13, 2017 17:16

addaleax added 2 commits September 13, 2017 20:44

src: make CleanupHandles() tear down handles/reqs

2ef5760

src: use cleanup hooks to tear down BaseObjects

99117a7

addaleax force-pushed the workers-impl branch from fb81652 to b4ae5fa Compare September 13, 2017 18:44

addaleax added 4 commits September 14, 2017 13:08

src: prepare v8 platform for multi-isolate support

6a48096

src: use lock for c-ares library init/cleanup

6741a6c

src: rename string id for _onclose to match

016f610

worker: implement MessagePort and MessageChannel

506836b

addaleax force-pushed the workers-impl branch 2 times, most recently from 5f8b13b to 56ceb5e Compare September 14, 2017 13:30

addaleax force-pushed the workers-impl branch from 56ceb5e to 36da346 Compare September 14, 2017 13:55

addaleax and others added 8 commits September 14, 2017 17:21

worker: initial implementation

19c6dcb

console: make global console work in workers

cfc86fa

test: add basic tests for MessageChannel

975656c

test: add basic tests for worker

87b2184

test: add more tests for workers

5168178

Taken from petkaantonov/io.js@ea143f7 and modified to fit current linter rules and coding style.

addons: integrate workers with native addons

d93dcf9

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

worker: implement vm.moveMessagePortToContext()

d3f3912

This should help a lot with actual sandboxing of JS code.

doc: add documentation for workers module

47e8463

addaleax force-pushed the workers-impl branch from 36da346 to 47e8463 Compare September 14, 2017 15:21

zkat closed this Sep 15, 2017

addaleax mentioned this pull request Sep 15, 2017

worker: initial implementation (large/base PR) #58

Closed

19 tasks

TimothyGu mentioned this pull request Sep 19, 2017

please develop native threads that can load native modules with require and share/lock objects #31

Open

bmeck mentioned this pull request Oct 7, 2017

Support .mjs output [also a TypeScript issue] TypeStrong/ts-node#436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: initial implementation #40

worker: initial implementation #40

addaleax commented Sep 2, 2017 •

edited

Loading

benjamingr Sep 2, 2017

addaleax Sep 2, 2017

benjamingr Sep 2, 2017 •

edited

Loading

addaleax Sep 2, 2017

benjamingr Sep 13, 2017

addaleax Sep 13, 2017

benjamingr Sep 15, 2017

benjamingr Sep 2, 2017 •

edited

Loading

addaleax Sep 2, 2017

benjamingr Sep 2, 2017

TimothyGu Sep 2, 2017

benjamingr Sep 2, 2017

benjamingr Sep 2, 2017

addaleax Sep 2, 2017

benjamingr Sep 2, 2017

addaleax Sep 2, 2017

TimothyGu Sep 2, 2017

benjamingr Sep 2, 2017

addaleax Sep 2, 2017

TimothyGu commented Sep 2, 2017 •

edited

Loading

benjamingr commented Sep 2, 2017

addaleax commented Sep 2, 2017

TimothyGu commented Sep 2, 2017

addaleax commented Sep 2, 2017

addaleax commented Sep 3, 2017 •

edited

Loading

addaleax commented Sep 3, 2017

TimothyGu commented Sep 4, 2017

addaleax commented Sep 5, 2017

addaleax commented Sep 13, 2017

Qard commented Sep 14, 2017

Qard commented Sep 14, 2017

addaleax commented Sep 14, 2017

addaleax commented Sep 15, 2017

worker: initial implementation #40

worker: initial implementation #40

Conversation

addaleax commented Sep 2, 2017 • edited Loading

Checklist

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 2, 2017 • edited Loading

benjamingr commented Sep 2, 2017

addaleax commented Sep 2, 2017

TimothyGu commented Sep 2, 2017

addaleax commented Sep 2, 2017

addaleax commented Sep 3, 2017 • edited Loading

addaleax commented Sep 3, 2017

TimothyGu commented Sep 4, 2017

addaleax commented Sep 5, 2017

addaleax commented Sep 13, 2017

Qard commented Sep 14, 2017

Qard commented Sep 14, 2017

addaleax commented Sep 14, 2017

addaleax commented Sep 15, 2017

addaleax commented Sep 2, 2017 •

edited

Loading

benjamingr Sep 2, 2017 •

edited

Loading

benjamingr Sep 2, 2017 •

edited

Loading

TimothyGu commented Sep 2, 2017 •

edited

Loading

addaleax commented Sep 3, 2017 •

edited

Loading