Add support for worker_threads

[sebamarynissen]

Since node 10.5 we have the option of using worker threads which have the advantage that they can use shared memory using SharedArrayBuffers. I've modified the worker-farm module to optionally support worker_threads. The api is completely the same and can be used like:

const workerFarm = require('worker-farm');
const service = workerFarm.threaded(require.resolve('/path/to/worker'));
// or
const service = workerFarm({worker_threads: true}, require.resolve('/path/to/worker'));

The worker_threads module is still experimental and should be explicitly enabled using the --experimental-worker flag. This is automatically detected, and if worker_threads is not available, it falls back to the default forked implementation, showing a warning that worker_threads is not available.

[rvagg]

Sweet! You'll have to be patient with me, I'm a bit busy this week but I'd love to get this landed and released. Feel free to ping me after this week if you don't hear from me again (overloaded with input so things tend to get forgotten). Initial look at the code is 👍 though.

[sebamarynissen]

Thanks, I'll keep that in mind.

In the meantime I've come to think that it should also be possible to pass in a transferList to efficiently pass data around between the worker thread and the main thread. The most obvious approach would be to do something like this:

// In the worker
module.exports = function(cb) {
  let arr = new Float32Array([Math.E]);
  let transferList = [arr.buffer];
  cb(null, arr, transferList);
};

The thing is that this would break the existing api, as currently in handle.js, all arguments passed in the callback are sent to the parent:

let _args = Array.prototype.slice.call(arguments);
send({ idx: idx, child: child, args: _args });

I think this will need to be changed to something like

let _args = Array.prototype.slice.call(arguments);
if (worker_threads) {
  send({ idx: idx, child: child, args: _args.slice(0,2) }, _args[2]);
}
else {
  send({ idx: idx, child: child, args: _args });
}

but this causes an api difference between worker_threads and the default child_process implementation.

@rvagg What's your opinion on this?

[rvagg]

I get why you've gone with this name but I think I'd prefer it be workerThreads

[rvagg]

break a newline here

[rvagg]

and swallow the blank line underneath

[rvagg]

send({ idx: idx, child: child, args: _args.slice(0,2) }, _args[2]) I think I'm OK with this since it doesn't break standard callback semantics but it's going to have to be clearly documented and will result in a breaking change. Transfer lists are certainly going to make this much more useful, without that feature I'm sure the issues will stack up asking for it. So go ahead and see how you go adding it.
Next we need documentation, care yo have a go at touching up the README?
I've cleared up a lot of the OSS backlog that's been pinning me down so should be much more responsive now. There's still quite a backlog here but this seems like as good a place as any to chip away at that.

[sebamarynissen]

I will see if I find some time this week to modify the code a bit to implement the TransferList & adapt the readme.

[sebamarynissen]

I've added the transferList functionality and modified the readme to document it.
I've also merged the onChild() functionality that was added in the meantime to work with worker_threads as well.

I did have to comment out some process.id related stuff in the tests though because they wouldn't pass otherwise. This is probably related to my Windows machine. Notably the

t.ok(pid > process.pid, 'pid makes sense')
t.ok(pid < process.pid + 750, 'pid makes sense')

tests did not pass.

[rvagg]

Yeah, that's OK on the PID tests, they're pretty specific to a particular implementation of PIDs that doesn't hold universally, even on Linuxes.
Thanks for the work, will have a look shortly and hopefully get this shipped!

[sebamarynissen]

One more thing: I didn't update the index.d.ts file because I don't know TypeScript, but it probably should be updated as well before shipping this.

[rvagg]

maybe @zeroEvidence can help us out on the typescript definitions for these changes

[xamgore]

Hey guys! That's a really nice feature, I'd like it to be released as soon as possible. Can I help somehow with type definitions?

[zeroEvidence]

maybe @zeroEvidence can help us out on the typescript definitions for these changes

Pardon for the delay, I've been sick.

I can give it a shot.

@rvagg May I recommend that you consider merging this to a dev branch so I can grab the changes from the original repo?

[sebamarynissen]

@zeroEvidence That would be great! Give me a shout if you need some help with figuring out the new api. The main difference is the addition of the .threaded() function to create a worker farm using worker_threads and the possibility to include transfer lists.
When calling the callback from the worker the transferList is the third argument

callback(err, result, transferList);

and when passing a transferList to the worker you specify the transferList after the callback

worker(...arguments, callback, transferList);

[sebamarynissen]

Given that node 12 - which doesn't require a flag for worker threads - entered LTS two days ago, I was wondering whether we shouldn't make the threaded implementation the default when we release it.

@rvagg, what is your opinion on this?

[ppiyush13]

Any updates on this pull request ?

[jdmarshall]

hey @rvagg how are we feeling about this PR?

I'd been looking to replace worker-farm with node workers but the main stumbling block is that the load balancing logic is pretty handy (Might possibly be handy as a separate module) and I came to ask the question that @sebamarynissen has already answered with this PR (what if we change worker-farm instead)

@sebamarynissen how have you been working around this PR getting stuck?

[sebamarynissen]

I use this in a production website of mine where I simply referred to my fork when specifying it as a dependency, like

{
  "dependencies": {
    "worker-farm": "git+https://github.com/sebamarynissen/node-worker-farm.git#feature/esm"
  }
}

Note that the branch I'm referring to is a more recent version where I refactored the code a bit more to my taste (using promises instead of callbacks), and also added the possibility to use es modules as the worker, given that currently only commonjs is supported. I never created a new PR for it though because it contained quite a few breaking changes.

[rvagg]

Hey @sebamarynissen, are you interested in, and can I trust you with, commit access to this project? I just don't use it and don't have the time to give it the care that it needs and there's a lot of people who still want to use it and want it in a better condition. Ideally we could find a few more people to collaborate and review each others' work too.

[jdmarshall]

@sebamarynissen Those would have made a good 2.0

Perhaps you should call this one 1.8 since it has one breaking change?

[sebamarynissen]

@rvagg, yeah I'm not sure. The project for which I initially made this PR got cancelled, and the production website I use it in is a small side project that I haven't looked into anymore for quite some time now. On top of that, I'm trying to start/run a business next to my main job, so I too feel that I cannot give the project the care that it needs.