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: handle calling terminate when kHandler is null #28370

Closed

Conversation

@elyalvarado
Copy link
Contributor

commented Jun 21, 2019

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 Outdated Show resolved Hide resolved

@elyalvarado elyalvarado force-pushed the elyalvarado:terminate-with-null-handlers branch from 77806ac to c59a793 Jun 22, 2019

@addaleax
Copy link
Member

left a comment

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

@TimothyGu
Copy link
Member

left a comment

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

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

@elyalvarado elyalvarado force-pushed the elyalvarado:terminate-with-null-handlers branch from c59a793 to 4222cd9 Jun 23, 2019

@elyalvarado

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

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

@elyalvarado 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

@khriztianmoreno
Copy link
Contributor

left a comment

Good job!

@addaleax

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

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

@nodejs-github-bot

This comment was marked as outdated.

@Trott
Trott approved these changes Jul 5, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

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

elyalvarado added 5 commits Jun 21, 2019
worker: handle calling terminate when kHandler is null
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).
worker: make calling terminate always return a promise
This change makes terminate always return a promise even if kHandler
is null
worker: always deprecation warning on terminate if callback supplied
The deprecation warning is printed even if the kHandler is null
Rename variable to reduce line length
...and make the linter happy

@Trott Trott force-pushed the elyalvarado:terminate-with-null-handlers branch from fd7f67e to 3916e59 Jul 30, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

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

@nodejs-github-bot

This comment has been minimized.

@elyalvarado

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

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

Trott added a commit to Trott/io.js that referenced this pull request Jul 31, 2019
worker: handle calling terminate when kHandler is null
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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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/.)

@Trott Trott closed this Jul 31, 2019

targos added a commit that referenced this pull request Aug 2, 2019
worker: handle calling terminate when kHandler is null
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>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
worker: handle calling terminate when kHandler is null
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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
worker: handle calling terminate when kHandler is null
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.