http2: adjust error emit in core, add tests #15586
Conversation
| break; | ||
| default: | ||
| // Some other unexpected error was reported. | ||
| if (ret < 0) { | ||
| err = new NghttpError(ret); | ||
| process.nextTick(() => this.emit('error', err)); | ||
| process.nextTick(emit, stream, 'error', err); |
There was a problem hiding this comment.
I checked the C++ code and I checked nghttp2 documentation and I believe this is supposed to emit on stream. Let me know if I'm wrong.
| break; | ||
| default: | ||
| // Some other unexpected error was reported. | ||
| if (ret < 0) { | ||
| err = new NghttpError(ret); | ||
| process.nextTick(() => this.emit('error', err)); | ||
| process.nextTick(emit, stream, 'error', err); |
There was a problem hiding this comment.
Same as above.
I checked the C++ code and I checked nghttp2 documentation and I believe this is supposed to emit on stream. Let me know if I'm wrong.
|
Thanks @jasnell! I appreciate the opportunity to help out. Should have more commits flowing in over the week as I've been able to get express to run with |
|
@BridgeAR ya I know. I might revisit trying to group some of the http2 tests in a separate PR. I'm not sure that it's particularly easy for these ones though. |
|
Could we get a CI started for this, please? Thanks! |
apapirovski
force-pushed
the
patch-http2-closures-and-tests
branch
from
1e6316e
to
25662fe
Compare
|
Looks like the CI was green. I've also rebased this and removed the Update: Copy & pasted the wrong commit message, force pushed again. |
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits.
apapirovski
force-pushed
the
patch-http2-closures-and-tests
branch
from
25662fe
to
1861beb
Compare
|
Landed in ccd3afc |
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits. PR-URL: #15586 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
Thanks Ruben! |
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits. PR-URL: #15586 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits. PR-URL: #15586 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits. PR-URL: nodejs/node#15586 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Use the ability of nextTick and setImmediate to pass arguments instead of creating closures or binding. Add tests that cover the vast majority of error emits. PR-URL: #15586 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This PR updates http2/core to need less bindings and closures when emitting various events, instead it uses the already existing ability of
process.nextTickandsetImmediateto pass through arguments. Despite having to use a rest parameter, this is actually faster than binding or creating a closure. My understanding is that with V8 6.2 this code will only get faster.(Also there's a minor fix for the fact that incorrect headers passed to the various respond with file methods weren't correctly exiting after an error and instead continued to run the rest of the function.)
I adjusted the existing
headersbenchmark to use a few more of the affected functions and the benchmark result is below (this is over 250 iterations for each node version). Not a huge difference but probably still worthwhile. (Theheadersbenchmark is the best for assessing these changes because it runs into these event emits on both the client and the server. I only usednheadersat 0 because the other variations stress the wrong parts of the system that we don't care about.)I've also added tests to cover most of these emits throughout the code. There are a few spots left uncovered but I'll need to figure out a slightly different way of testing than just mocking the handle method to output error codes. I don't think that should hold up this PR though as they were already not covered by tests.
Thanks for reviewing!
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http2, test