FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

worker: display MessagePort status in util.inspect() by addaleax · Pull Request #22658 · nodejs/node · GitHub

/ node Public

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

Closed
addaleax wants to merge 6 commits into
nodejs:masterfrom
addaleax:message-port-inspect
ConversationCommits (6)ChecksFiles changed
Closed

worker: display MessagePort status in util.inspect() #22658
addaleax wants to merge 6 commits into
nodejs:masterfrom
addaleax:message-port-inspect

Conversation

addaleax commented Sep 2, 2018

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

addaleax added the worker Issues and PRs related to Worker support. label Sep 2, 2018

nodejs-github-bot commented Sep 2, 2018

Copy link
Copy Markdown
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 added the worker Issues and PRs related to Worker support. label Sep 2, 2018

addaleax commented Sep 2, 2018

Copy link
Copy Markdown
Member Author

@nodejs/workers

benjamingr approved these changes Sep 2, 2018
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice!

addaleax commented Sep 2, 2018

Copy link
Copy Markdown
Member Author

Fixed linter nits.

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 2, 2018
lundibundi approved these changes Sep 2, 2018
View reviewed changes

lundibundi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a few nits.

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

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

lundibundi Sep 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is return needed here?

addaleax Sep 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No, removed it :)

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

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

lundibundi Sep 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Sep 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nope. Thanks for catching!

Comment thread lib/internal/worker.js Outdated
} : {
active: true,
refed: ref
}, this);

lundibundi Sep 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

addaleax Sep 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done (moving to a separate line)

addaleax commented Sep 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

addaleax commented Sep 5, 2018

Copy link
Copy Markdown
Member Author

Landed in caf8e7a, b3eb3a1

addaleax closed this Sep 5, 2018
addaleax deleted the message-port-inspect branch September 5, 2018 10:31
addaleax added a commit that referenced this pull request Sep 5, 2018
worker: display MessagePort status in util.inspect()
caf8e7a 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Sep 5, 2018
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 <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 5, 2018
worker: display MessagePort status in util.inspect()
1daa82a 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 5, 2018
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 <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
worker: display MessagePort status in util.inspect()
ed96c3c 
PR-URL: #22658
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 6, 2018
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 <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Sep 6, 2018
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
lundibundi lundibundi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL