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: prevent event loop starvation through MessagePorts #28030

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 2, 2019

Limit the number of messages processed without interruption on a
given MessagePort to prevent event loop starvation.

This aligns the behaviour better with the web.

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

Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation.

This aligns the behaviour better with the web.
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Jun 2, 2019
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Just curious: is it from a case in WPT?

src/node_messaging.cc Show resolved Hide resolved
@addaleax
Copy link
Member Author

addaleax commented Jun 2, 2019

@joyeecheung
Copy link
Member

@addaleax cool, do you have an order to go through the tests? We talked about auditing the WPT and port as much as possible before moving workers out of experimental, so some kind of checklist might be helpful to track the progress.

(I can probably script some checklist using node-core-utils, if you like)

}

port2.postMessage(0);
assert(count++ < 10000, `hit ${count} loop iterations`);
Copy link
Member

@joyeecheung joyeecheung Jun 3, 2019

Choose a reason for hiding this comment

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

It may be useful to document that this depends on the constant in the source - in case someone wants to turn it down later. Or we could also export the constant and do some math here..

(Non-blocking BTW)

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. I don't think a named constant is necessary, it's pretty obvious what it does.

@addaleax
Copy link
Member Author

addaleax commented Jun 4, 2019

@addaleax cool, do you have an order to go through the tests? We talked about auditing the WPT and port as much as possible before moving workers out of experimental, so some kind of checklist might be helpful to track the progress.

I’ve gone through all the files in the order in which my editor opened them, and opened PRs along the way 🙂 There’s one left, and it may require a (slightly?) different approach here, because the current queue of messages should still end up being processed even if one of the message handlers closes the port; and this PR would break that, because after closing the port, TriggerAsync(); does not work anymore.

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Jun 7, 2019
@addaleax addaleax closed this Aug 25, 2019
@addaleax addaleax deleted the message-port-event-loop-starvation branch August 25, 2019 15:56
addaleax added a commit to addaleax/node that referenced this pull request Aug 25, 2019
Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation, but still
make sure that all messages are emitted that were already in the
queue when emitting began.

This aligns the behaviour better with the web.

Refs: nodejs#28030
addaleax added a commit to addaleax/node that referenced this pull request Sep 7, 2019
Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation, but still
make sure that all messages are emitted that were already in the
queue when emitting began.

This aligns the behaviour better with the web.

Refs: nodejs#28030
addaleax added a commit that referenced this pull request Sep 9, 2019
Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation, but still
make sure that all messages are emitted that were already in the
queue when emitting began.

This aligns the behaviour better with the web.

Refs: #28030

PR-URL: #29315
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2019
Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation, but still
make sure that all messages are emitted that were already in the
queue when emitting began.

This aligns the behaviour better with the web.

Refs: #28030

PR-URL: #29315
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Limit the number of messages processed without interruption on a
given `MessagePort` to prevent event loop starvation, but still
make sure that all messages are emitted that were already in the
queue when emitting began.

This aligns the behaviour better with the web.

Refs: #28030

PR-URL: #29315
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants