-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Conversation
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.
Landed in f284290...78743f8 |
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
@addaleax if this is something that should be in |
This is a follow-up to #31386, so I’ll try to backport those together |
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:
|
@apapirovski This is about the C++ |
@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.) |
@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 |
@addaleax Yeah, that makes sense to me. I guess that was the original crux of the issue — the naming ties into the JS |
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Refs: #31502 (comment) PR-URL: #32300 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #31502 (comment) PR-URL: #32300 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Refs: #31502 (comment) PR-URL: #32300 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 likeprocess.nextTick()
(which matches how we use it) by also running tasks that have been
added during previous
SetImmediate()
calls.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes