Skip to content

worker: refactor `worker.terminate()`

Rodrigo Muino Tomonari requested to merge github/fork/addaleax/worker-terminate into master

At the collaborator summit in Berlin, the behaviour of worker.terminate() was discussed.

In particular, switching from a callback-based to a Promise-based API was suggested. While investigating that possibility later, it was discovered that .terminate() was unintentionally synchronous up until now (including calling its callback synchronously).

Also, the topic of its stability has been brought up. I have performed two manual reviews of the native codebase for compatibility with .terminate(), and performed some manual fuzz testing with the test suite. At this point, bugs with .terminate() should, in my opinion, be treated like bugs in other Node.js features. (It is possible to make Node.js crash with .terminate() by messing with internals and/or built-in prototype objects, but that is already the case without .terminate() as well.)

This commit:

  • Makes .terminate() an asynchronous operation.
  • Makes .terminate() return a Promise.
  • Runtime-deprecates passing a callback.
  • Removes a warning about its stability from the documentation.
  • Eliminates an unnecessary extra function from the C++ code.

A possible alternative to returning a Promise would be to keep the method synchronous and just drop the callback. Generally, providing an asynchronous API does provide us with a bit more flexibility.

Refs: https://github.com/nodejs/summit/issues/141

@nodejs/workers @benjamingr

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

Merge request reports