worker: handle calling terminate when kHandler is null by elyalvarado · Pull Request #28370 · nodejs/node

elyalvarado · 2019-06-21T22:30:18Z

This PR makes a change to the Worker.terminate() method when
called if the kHandler is null. Before this pull request it was returning
undefined, but the API is expecting a promise. With the changes in
this PR if terminate is called and kHandler is null a resolved Promise
is returned even if a callback is passed, to be consistent with the API
surface, and with always return promises.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

lib/internal/worker.js

addaleax

I think you need to run make lint and address the errors, but otherwise this looks good to me :)

TimothyGu

LGTM other than this comment and @addaleax’s :)

lib/internal/worker.js

elyalvarado · 2019-06-23T22:36:29Z

Pushed the following changes:

make lint now passing, this included rewording the commit messages to include the subsystem
Changed the return statements to print the deprecation warning if a callback is passed, and still return a promise if kHandle is null

khriztianmoreno

Good job!

addaleax · 2019-07-04T16:56:40Z

CI (without rebasing due to the merge commit): https://ci.nodejs.org/job/node-test-pull-request/24264/

nodejs-github-bot · 2019-07-05T01:53:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24278/

test/parallel/test-worker-terminate-null-handler.js

nodejs-github-bot · 2019-07-15T14:29:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24468/

nodejs-github-bot · 2019-07-30T04:14:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24765/

nodejs-github-bot · 2019-07-30T05:14:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24771/

Trott · 2019-07-30T05:52:55Z

Can you remove the merge commit? Our Ci does not handle them well.

This PR makes a change to the Worker.terminate() when called if the kHandler is null. Before this pull request it was returning undefined, but the API is expecting a promise. With the changes in this PR if terminate is called a Promise.resolve() is returned, unless a callback is passed in which case the old behavior stays (returns undefined).

This change makes terminate always return a promise even if kHandler is null

The deprecation warning is printed even if the kHandler is null

...and make the linter happy

Trott · 2019-07-30T23:38:12Z

Rebased against master and force pushed to eliminate the merge commit so we can run Ci and get this landed....

nodejs-github-bot · 2019-07-30T23:39:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24789/

elyalvarado · 2019-07-31T02:10:23Z

Thanks @Trott I was precisely going to ask how should I eliminate the merge commit. Now I know 😉. You all rock 🎸

This PR makes a change to the Worker.terminate() when called if the kHandler is null. Before this pull request it was returning undefined, but the API is expecting a promise. With the changes in this PR if terminate is called a Promise.resolve() is returned, unless a callback is passed in which case the old behavior stays (returns undefined). PR-URL: nodejs#28370 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2019-07-31T02:44:33Z

Landed in 9083a67.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

This PR makes a change to the Worker.terminate() when called if the kHandler is null. Before this pull request it was returning undefined, but the API is expecting a promise. With the changes in this PR if terminate is called a Promise.resolve() is returned, unless a callback is passed in which case the old behavior stays (returns undefined). PR-URL: #28370 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the worker Issues and PRs related to Worker support. label Jun 21, 2019

TimothyGu reviewed Jun 22, 2019

View reviewed changes

lib/internal/worker.js Outdated Show resolved Hide resolved

elyalvarado force-pushed the terminate-with-null-handlers branch from 77806ac to c59a793 Compare June 22, 2019 12:33

addaleax approved these changes Jun 22, 2019

View reviewed changes

TimothyGu approved these changes Jun 22, 2019

View reviewed changes

lib/internal/worker.js Outdated Show resolved Hide resolved

jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jun 23, 2019

elyalvarado force-pushed the terminate-with-null-handlers branch from c59a793 to 4222cd9 Compare June 23, 2019 22:34

elyalvarado changed the title ~~handle calling terminate on a worker when kHandler is null~~ worker: handle calling terminate when kHandler is null Jun 23, 2019

TimothyGu approved these changes Jun 23, 2019

View reviewed changes

khriztianmoreno approved these changes Jul 4, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 4, 2019

This comment has been minimized.

Sign in to view

BridgeAR approved these changes Jul 4, 2019

View reviewed changes

Trott approved these changes Jul 5, 2019

View reviewed changes

jasnell reviewed Jul 15, 2019

View reviewed changes

test/parallel/test-worker-terminate-null-handler.js Outdated Show resolved Hide resolved

jasnell approved these changes Jul 17, 2019

View reviewed changes

elyalvarado added 5 commits July 30, 2019 16:36

worker: make calling terminate always return a promise

fd8a604

This change makes terminate always return a promise even if kHandler is null

worker: always deprecation warning on terminate if callback supplied

0cb0c9c

The deprecation warning is printed even if the kHandler is null

Rename variable for clarity

f07fd01

Rename variable to reduce line length

3916e59

...and make the linter happy

Trott force-pushed the terminate-with-null-handlers branch from fd7f67e to 3916e59 Compare July 30, 2019 23:38

Trott closed this Jul 31, 2019

BridgeAR mentioned this pull request Aug 6, 2019

v12.8.0 release proposal #29017

Merged

Uh oh!

Conversation

elyalvarado commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elyalvarado commented Jun 23, 2019

Uh oh!

khriztianmoreno left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 4, 2019

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jul 5, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 15, 2019

Uh oh!

nodejs-github-bot commented Jul 30, 2019

Uh oh!

nodejs-github-bot commented Jul 30, 2019

Uh oh!

Trott commented Jul 30, 2019

Uh oh!

Trott commented Jul 30, 2019

Uh oh!

nodejs-github-bot commented Jul 30, 2019

Uh oh!

elyalvarado commented Jul 31, 2019

Uh oh!

Trott commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

elyalvarado commented Jun 21, 2019 •

edited

Loading