Discussion: Criteria for upgrading worker_threads module from experimental to stable

[Trott]

• User-space modules built on worker_threads
  • worker-threads-pool
  • microjob
  • node-worker-threads-pool
• Modules in citgm that exercise worker_threads in tests
  • ...
• Reports of worker_threads deployed in production
  • ...
• Framework adoption
  • ...
• full test coverage
  • test coverage is now 100% for worker_threads JS files in core
• No flaky tests
  • fix flaky test/parallel/parallel.status:test-worker-debug (Investigate flaky parallel/test-worker-debug on Windows #28106)
  • fix flaky test/parallel/parallel.status:test-worker-prof
  • fix flaky test/parallel/parallel.status:test-worker-syntax-error
  • fix flaky test/parallel/parallel.status:test-worker-syntax-error-file
  • fix flaky test/parallel/parallel.status:test-worker-memory
• Review source code for compatibility with terminate()
• No reliance on experimental V8 APIs (ValueSerializer in particular)

/cc @nodejs/workers

[AyushG3112]

Question: Is the API more or less frozen?

[devsnek]

Would be nice to see locks be stable by then too, but maybe that's a separate thing to track?

[addaleax]

@devsnek I’d track that separately, given that it’s a fully separate API and is useful independently of Workers?

[joyeecheung]

Question: Is the API more or less frozen?

From Node's side it's still experimental so I believe it's not frozen, but it's related to Web Workers which has a stable spec and we (kind of? correct me if I am wrong) agreed in nodejs/TSC#557 that we don't want to deviate too far from the existing design.

[addaleax]

Fwiw, there’s also the issue of workers not playing well with fs stat watchers because their cleanup code calls its callback prematurely on the libuv side (libuv/libuv#1869) … should things like that go on the list?

[Trott]

should things like that go on the list?

I guess if we don't want it to be stable before that's fixed, then yes.

[devsnek]

just reading the microjob readme... So, Microjob treats Node.js threads as temporary working units: if you need to spawn a long-living thread, then you should...

I think another thing to block on is better documentation. people should be re-using threads as much as possible.

[benjamingr]

@KromDaniel

[benjamingr]

Does the inspector work already? (I was away and didn't notice)

Would also be interested in getting sockets transferable - @KromDaniel has expressed interest in working on that (although I'm definitely not blocking on this)

[addaleax]

Does the inspector work already? (I was away and didn't notice)

It should, yes. :)

Would also be interested in getting sockets transferable

I wouldn’t list that as a criterion here, but I’d also love to see that! Adressing libuv/libuv#390 would be the “hard” part here.

[stevenvachon]

https://github.com/mvcbox/node-function-thread is interesting as well for running a single function in a thread. Perhaps not low-level enough and could be rewritten to use the new APIs.

[empyrical]

One thing I saw in one of the original discussions was that the module couldn't be called worker because the owner of that package name on NPM wanted to keep the name. Would workers be an acceptable option? Before moving out of experimental might be a good time to change the name to something shorter (and leaving worker_threads as an alias to the new name, because there are already user modules that use it)

cc @romansky: would you be open to letting the Node.js project use the package name workers for this? ( https://www.npmjs.com/package/workers )

[guybedford]

I'd like it if there could be an assessment done from a web compatibility and tooling perspective wrt to workflows around writing universal multi-threaded code.

I imagine it would be straightforward to write a browser wrapper for worker_threads, or simply have some conditional worker instantiation code between browser and Node, while sharing the main body, but I haven't gone into these details too deeply. I'd like to know someone has though, so that if there are any workflow hindrances that put a spanner in the tooling works, that these don't come up late in the day after the stability badge has long past.

I completely understand too if this is already decided as a non-goal as well, and will gladly have that discussion too.

If I've missed any of these points in previous discussions please do point out where I can read further.

[romansky]

I'd be happy for you to have it. Let me know how to do the transfer

…

On Fri, Oct 5, 2018, 2:27 AM empyrical ***@***.***> wrote: One thing I saw in one of the original discussions was that the module couldn't be called worker because the owner of that package name on NPM wanted to keep the name. Would workers be an acceptable option? Before moving out of experimental might be a good time to change the name to something shorter (and leaving worker_threads as an alias to the new name, because there are already user modules that use it) cc @romansky <https://github.com/romansky>: would you be open to letting the Node.js project use the package name workers for this? ( https://www.npmjs.com/package/workers ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlqAWU16g9omNZz6eIGTjJmT6pZhKzUks5uhplrgaJpZM4Wva44> .

[benjamingr]

@romansky are you coming to Reversim on Monday by any chance (saw Hasadna in orgs <3)? If so - can I buy you coffee to thank you in person?

[romansky]

Thank you Benhamin! I'm traveling on Tuesday so not sure about Monday..

…

On Sun, Oct 7, 2018 at 10:24 AM Benjamin Gruenbaum ***@***.***> wrote: @romansky <https://github.com/romansky> are you coming to Reversim on Monday by any chance (saw Hasadna in orgs <3)? If so - can I thank you in person? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlqAY89kw4NnyCxIreMtMWXDSzG5wbfks5uiaw_gaJpZM4Wva44> .

-- --- "Make everything as simple as possible, but not simpler." - Albert Einstein

[benjamingr]

@romansky ok, if you'd like to get coffee I'll be there (likely with a Node shirt) - so if you're up for it reach out (it's welcome) and if not some other time :)

[targos]

Aren't we supposed to use the @nodejs/ namespace for new modules?

[benjamingr]

(I was just happy about the gesture - not voicing an opinion about actually using the name either way)

[empyrical]

@romansky You can add someone as an owner to your module from the CLI like this:

npm owner add USERNAME workers

Replace USERNAME with the name of the account you'd like to transfer it to. I do not know who it should be transferred to, however. @benjamingr, is your npm name also benjamingr?

Also, I guess discussion whether or not the package should be renamed could be done as a new issue

[devsnek]

+100 for @nodejs prefix

[romansky]

@benjamingr I added you as a maintainer via the UI, hope that is what you are looking for. Feel free to use it and of-course remove me as a member for that NPM package.

…

On Sun, Oct 7, 2018 at 9:08 PM Gus Caplan ***@***.***> wrote: +100 for @nodejs prefix — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlqAbdRG8dqFOeXwMdo8-YOYUvK-ceqks5uikMmgaJpZM4Wva44> .

-- --- "Make everything as simple as possible, but not simpler." - Albert Einstein

[benjamingr]

Thanks @romansky !

benjamingr is me but it's an old account I don't have access to - I use benjamin.gruenbaum or add Anna (https://www.npmjs.com/~addaleax) or the foundation account ( https://www.npmjs.com/~nodejs-foundation ) :)

[slonka]

Hi everyone,

I would like to inform you that worker_threads landed in node-worker-nodes some time ago ( allegro/node-worker-nodes#29 ) and was successfully tested in a large scale environment (Allegro is the biggest online marketplace in Poland). I would also like to work on some of the issues mentioned here (mostly full test coverage and fixing flaky tests) as a part of hacktoberfest and I would welcome mentorship on those issues.

[romansky]

Permissions granted!

…

On Thu, Oct 11, 2018 at 6:05 PM Benjamin Gruenbaum ***@***.***> wrote: Thanks @romansky <https://github.com/romansky> ! benjamingr *is* me but it's an old account I don't have access to - I use benjamin.gruenbaum or add Anna (https://www.npmjs.com/~addaleax) or the foundation account ( https://www.npmjs.com/~nodejs-foundation ) :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlqAV-Ge8BWEqqHWl2_m6q0tbn5joKCks5uj15DgaJpZM4Wva44> .

-- --- "Make everything as simple as possible, but not simpler." - Albert Einstein

[alexvy86]

I already have that set, passing it to the node process as in your first example. I thought maybe something had to be done when spawning the workers. I remember when using the child_process module that execArgv (for the child processes) needs to be set manually in certain scenarios in order to be able to debug the children, so I thought maybe something similar needed to be done for worker_threads, but can't find any documentation about it...

[addaleax]

@nodejs/v8 Any ideas on when the ValueSerializer API might exit experimental state?

[addaleax]

@hashseed Maybe you could figure out more about the state of ValueSerializer?

[addaleax]

(Maybe @jeremyroman?)

[hashseed]

I don't think the ValueSerializer is to be considered experimental. It is used in production code and undergoes fuzz testing.

Also, sorry for the late response. Somehow slipped my inbox.

[targos]

@hashseed I think Anna was asking because of the warning comments in v8.h:

https://github.com/v8/v8/blob/cc7ac98b0c02ea6c98035194c86068cb873dd106/include/v8.h#L2007-L2015

https://github.com/v8/v8/blob/cc7ac98b0c02ea6c98035194c86068cb873dd106/include/v8.h#L2131-L2139

[addaleax]

Yeah – I’m happy to submit a CL to remove it, if it’s no longer accurate (the last API change was over a year ago, so I think that should be okay?).

Edit: Opened https://chromium-review.googlesource.com/c/v8/v8/+/1461999 after Yang’s comment below.

[hashseed]

Please submit a CL. I'll get the right people to review this.

[jeremyroman]

Replied on that CL; LGTM. ValueSerializer is pretty stable at this point, though the format is still quirky in a way that may make it inconvenient for some potential uses. Messaging to in-process workers is, of course, one of the things Chromium does use it for. :)

[trevnorris]

I'd like to add #25933 to the list. Worker should be able to gracefully handle OOM on initialization by emitting an error, instead of aborting. I came across this when doing memory consumption stress tests on the Worker. The specifics are in the linked issue.

[addaleax]

@trevnorris #25933 is a problem that is only partially specific to Workers, so I would be reluctant to put it on the list. V8 (and therefore Node.js) has no good handling for out-of-memory situations and can crash as a consequence of that – with or without Workers.

[trevnorris]

@addaleax At first I questioned whether this should be on the list, but after considering how it's affecting Workers I decided it should be added. After the Worker is initialized and the user's code is running there is the option to hook into v8::Isolate::SetOOMErrorHandler(), along with other utilities to monitor and check the health of the thread. This isn't the case during initialization.

There is no method to check whether a Worker will abort before it's created, e.g. simply looking at available memory isn't sufficient (such as my uname -v example in the issue). Because there is a lack of visibility into Worker initialization I believe Node should be able to guarantee it won't abort until the user is able to take control.

[addaleax]

After the Worker is initialized and the user's code is running there is the option to hook into v8::Isolate::SetOOMErrorHandler()

That doesn’t get us much, as this also leads to unconditional aborts when it is called.

There is no method to check whether a Worker will abort before it's created, e.g. simply looking at available memory isn't sufficient (such as my uname -v example in the issue). Because there is a lack of visibility into Worker initialization I believe Node should be able to guarantee it won't abort until the user is able to take control.

My point was that this is true for every other type of Node.js object as well, Workers might just be one of the more resource-intensive ones.

[sam-github]

FYI: nodejs/build#1840 (comment), test-worker-debug may be flaky on arm and need looking into.

[kaushalyap]

Any plans to upgrade to stable?

[addaleax]

@kaushalyap In May at our Collaborator Summit we agreed that the remaining criterion before upgrading to stable would be making sure that our Web Messaging implementation matches with the Web Platform Tests for those, so I think at this point it’s only fixing #29315 to work on Windows and then we’re good to go. (It’s probably not a lot of work, but debugging on Windows sadly still takes a lot of time and effort, at least for me.)

[carera]

Hi all,
Thank you everyone for the awesome work around worker threads!

I am not entirely sure how to ask this, but still worth the try: When I am trying to profile an app using V8 profiler, the job executed on worker_threads does not show up in the profiles. I am using this lib: https://github.com/hyj1991/v8-profiler-next which exposes node bindings inside JS.
(see my issue: hyj1991/v8-profiler-next#9)
Is there a plan to make this work, when worker_threads are considered stable?
Thank you so much.
Cheers! 💜

[benjamingr]

@carera hey, first of all thank you for chiming in :]

In hyj1991/v8-profiler-next#9 (comment) it appears that slonka has confirmed that worker_threads are supported. I have also been able to profile them in the past when testing PRs related to them.

Just wondering - why are you invoking the profiler directly rather than going through the inspector and the debugger protocol? You should be able to open an inspector and do something like Profiler.enable and then call Profiler.start to start profiling and Profiler.end to stop. Do the the inspector docs help?

I think (I don't remember precisely but remember vaguely from reviewing but I can happily check for you) there should be something like a NodeRuntime domain that lets you switch to worker threads and that tools like ndb should support profiling worker threads out of the box.

There are a few things to keep in mind:

• Profiling multi-threaded applications well is in my opinion a lot more than just profiling each thread independently (which is what currently works).
• We would need help from the V8 team to enable that and they would probably need to build those tools for web-workers first.

Mostly - we have been waiting for people to be engaged and raise what they want and use cases.

[carera]

Thank you for your swift response, @benjamingr. The use case we have is the ability to profile on demand in production without the need of running the service with any --prof or debugger flags.
That is mostly because we fear that these profiling flags have performance impact and they also generate files that continuously grow.
V8-profiler seemed to be a great idea as we could turn the profiling on/off for desired periods, such as during a production incident.

[benjamingr]

@carera You would open the inspector dynamically, then call Profiler.enable - that can happen dynamically and not incur a performance impact and you could even do this (with ndb for example) with a running Node.js process in production :]

[carera]

Thank you @benjamingr again! I am not entirely sure how does ndb fall into the picture, as I am not familiar with it, but the inspector looks promising! I am gonna give it a go.
Thank you again and sorry for possibly sidetracking from this issue topic :)
Love y'all! 💜

[benjamingr]

@carera ndb just has built in support for debugging worker threads. You would send a SIGUSR to the process which would open the debugger than use ndb to connect to that (since that is less manual than using the debugger protocol yourself with the built in inspector module) and then that should automatically let you debug and profile worker_threads.