Skip to content
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: refactor worker.terminate() #28021

Closed
wants to merge 1 commit into from

Commits on Jun 10, 2019

  1. worker: refactor worker.terminate()

    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: openjs-foundation/summit#141
    addaleax committed Jun 10, 2019
    Configuration menu
    Copy the full SHA
    34efb18 View commit details
    Browse the repository at this point in the history