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.

Sign up for GitHub

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

Jump to bottom

worker: display MessagePort status in util.inspect() #22658

Closed
wants to merge 6 commits into from
Closed

worker: display MessagePort status in util.inspect() #22658

wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 2, 2018

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

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Sep 2, 2018
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/772/pipeline

@nodejs-github-bot nodejs-github-bot added the worker Issues and PRs related to Worker support. label Sep 2, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 2, 2018

@nodejs/workers

benjamingr
benjamingr approved these changes Sep 2, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@addaleax
Copy link
Member Author

addaleax commented Sep 2, 2018

Fixed linter nits.

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2018
lundibundi
lundibundi approved these changes Sep 2, 2018
View reviewed changes
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few nits.

test/parallel/test-worker-message-port-transfer-self.js Outdated

function tick(n, cb) {
if (--n > 0)
return setImmediate(() => tick(n - 1, cb));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is return needed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, removed it :)

test/parallel/test-worker-message-port-transfer-self.js Outdated
});
port1.postMessage(2);

function tick(n, cb) {
if (--n > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually makes half-the-n ticks, was this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Thanks for catching!

lib/internal/worker.js Outdated
} : {
active: true,
refed: ref
}, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could you please put this on a separate line as it's kind of confusing?
Also, I'd suggest extracting second argument creation to a separate variable to make it easier to understand =). Though I'm fine with this too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (moving to a separate line)

@addaleax
Copy link
Member Author

addaleax commented Sep 2, 2018 •
edited
Loading

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16975/ (:heavy_check_mark:)

@addaleax
Copy link
Member Author

addaleax commented Sep 5, 2018

Landed in caf8e7a, b3eb3a1

@addaleax addaleax closed this Sep 5, 2018
@addaleax addaleax deleted the message-port-inspect branch September 5, 2018 10:31
addaleax added a commit that referenced this pull request Sep 5, 2018
@addaleax
worker: display MessagePort status in util.inspect()
caf8e7a 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax added a commit that referenced this pull request Sep 5, 2018
@addaleax
test: fix flaky test-worker-message-port-transfer-self
b3eb3a1 
Look at the status of the `MessagePort` rather than
relying on a timeout.

PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 5, 2018
@addaleax @targos
worker: display MessagePort status in util.inspect()
1daa82a 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 5, 2018
@addaleax @targos
test: fix flaky test-worker-message-port-transfer-self
c26747d 
Look at the status of the `MessagePort` rather than
relying on a timeout.

PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@addaleax @targos
worker: display MessagePort status in util.inspect()
ed96c3c 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
@addaleax @targos
test: fix flaky test-worker-message-port-transfer-self
8e4caf5 
Look at the status of the `MessagePort` rather than
relying on a timeout.

PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @nodejs-github-bot @jasnell @benjamingr @lpinca @TimothyGu @BridgeAR @lundibundi