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

src,http2: simplify native immediate queue running and harden http2 test #31502

Closed

Conversation

addaleax
Copy link
Member

test: make test-http2-buffersize more correct

Previously, this code could have closed the server before the
connection was actually received by the server, as the 'close'
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

src: simplify native immediate queue running

Make SetImmediate() behave more like process.nextTick()
(which matches how we use it) by also running tasks that have been
added during previous SetImmediate() calls.

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

Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 25, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 26, 2020

@addaleax
Copy link
Member Author

Landed in f284290...78743f8

@addaleax addaleax closed this Jan 27, 2020
addaleax added a commit that referenced this pull request Jan 27, 2020
Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
addaleax added a commit that referenced this pull request Jan 27, 2020
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@addaleax addaleax deleted the http2-setimmediate-buffersize branch January 27, 2020 17:17
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere
Copy link
Member

@addaleax if this is something that should be in v12.x, could you please backport it? There are a nest of conflicts and i want to be extra careful 😅

@addaleax
Copy link
Member Author

This is a follow-up to #31386, so I’ll try to backport those together

@apapirovski
Copy link
Member

I missed this originally @addaleax but this seems like a semver-major to me, so probably shouldn't be backported? See https://nodejs.org/api/timers.html#timers_setimmediate_callback_args where we document the setImmediate behavior as:

If an immediate timer is queued from inside an executing callback, that timer will not be triggered until the next event loop iteration.

@addaleax
Copy link
Member Author

@apapirovski This is about the C++ SetImmediate(), it has no effect on the JS API

@apapirovski
Copy link
Member

apapirovski commented Mar 15, 2020

@addaleax yup, I get that but the queues are somewhat tied to each other, right? Or well, at least I would expect them to be given the naming... (They certainly are tied in terms of execution timing, at the very least.)

@addaleax
Copy link
Member Author

@apapirovski The queues are managed independently, no connection there.

The naming is the same because they use the same underlying mechanism, but effectively we use SetImmediate() as nextTick() in the C++ source code. I’d be okay with renaming it to something else, just not NextTick() – while that would be 100 % accurate, it’s not true for the JS nextTick() and that might be even more confusing ;)

@apapirovski
Copy link
Member

@addaleax Yeah, that makes sense to me. I guess that was the original crux of the issue — the naming ties into the JS setImmediate but the behavior only partially does. That said, I also don't have any great ideas re: naming. Maybe it's just a matter of adding a comment to the native SetImmediate function that calls this difference out?

addaleax added a commit to addaleax/node that referenced this pull request Mar 16, 2020
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
jasnell pushed a commit that referenced this pull request Mar 21, 2020
Refs: #31502 (comment)

PR-URL: #32300
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Refs: #31502 (comment)

PR-URL: #32300
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Previously, this code could have closed the server before the
connection was actually received by the server, as the `'close'`
event on the client side can be emitted before the connection
is established.

The following commit exacerbates this problem, so fix the test first.

PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.

PR-URL: nodejs#31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.

Backport-PR-URL: #32301
PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
Refs: #31502 (comment)

PR-URL: #32300
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@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
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants