Currently, writeQueueSize is never used in C++ and barely used
within JS. Instead of constantly updating the value on the JS
object, create a getter that will retrieve the most up-to-date
value from C++.
For the vast majority of cases though, create a new prop on
Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this
to track the current write size, entirely in JS land.
PR-URL: https://github.com/nodejs/node/pull/17650
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Remove length prop on NextTickQueue class. We technically don't need to
keep track of the length of the queue in two places as we already have
tickInfo doing that work (between the index & the length we have enough
data for everything).
Store asyncId in a const within the _tickCallback function. Accessing
Symbol properties seems to be quite a bit more expensive than string
keys so this actually has a decent performance impact.
PR-URL: https://github.com/nodejs/node/pull/17421
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Remove micro-optimizations that no longer yield any benefits,
restructure timers & immediates to be a bit more straightforward.
Adjust timers benchmarks to run long enough to offer meaningful data.
PR-URL: https://github.com/nodejs/node/pull/17279
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Do not call destroy each time rstStream is called since the
first call (or receipt of rst frame) will always trigger
destroy. Expand existing test for this behaviour.
PR-URL: https://github.com/nodejs/node/pull/16753
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Two instances of a similar test exist, with both testing the timeout on
the stream and neither on the session. Adjust one of them to test the
session timeout instead.
PR-URL: https://github.com/nodejs/node/pull/16754
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
No longer check whether key is a symbol as Object.keys does not
return symbols. No longer convert key to string as it is always
a string. Validate that only one value is passed for each
pseudo-header.
Extend illegal connection header message to include the name of
the problematic header.
Extend tests to cover this behaviour.
PR-URL: https://github.com/nodejs/node/pull/16575
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* correctly reset write timers: currently reset timers on
both session & stream when write starts and when it ends.
* prevent large writes from timing out: when writing a large
chunk of data in http2, once the data is handed off to C++,
the JS session & stream lose all track of the write and will
timeout if the write doesn't complete within the timeout window
Fix this issue by tracking whether a write request is ongoing and
also tracking how many chunks have been sent since the most recent
write started. (Since each write call resets the timer.)
PR-URL: https://github.com/nodejs/node/pull/16525
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
When compiling with --debug-http2 flag, compiler complains
about passing wrong type of argument to DEBUG_HTTP2. Fix
by using static_cast to uint32_t.
PR-URL: https://github.com/nodejs/node/pull/16373
Reviewed-By: James M Snell <jasnell@gmail.com>
options.getTrailers & options.selectPadding will always be a
function by the time the code reaches the assert checks. And
even if not (which seems currently impossible), the assert
failure will be almost the same as calling something that is
not a function. The downside of leaving these is that typeof
checks are decently expensive.
PR-URL: https://github.com/nodejs/node/pull/16327
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Minimize property access via Symbols by consistently creating
new constants for things that are retrieved multiple times
within a method or a function. Fix cases where existing
constants aren't used where appropriate.
PR-URL: https://github.com/nodejs/node/pull/16327
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Due to changes in V8 in 6.0 & 6.1, it's no longer necessary to copy
arguments to avoid deopt. Just call .apply with arguments. Retains
fast cases for 0-3 arguments.
events/ee-once-4-args.js n=20000000 11.58 % *** 1.310379e-05
PR-URL: https://github.com/nodejs/node/pull/16212
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
With recent changes in V8, it is now as performant or faster to use
spread parameter within EventEmitter.prototype.emit, especially
in cases where looping over arguments is required.
events/ee-emit.js n=2000000 4.40 % *** 1.505543e-06
events/ee-emit-1-arg.js n=2000000 2.16 % *** 2.434584e-10
events/ee-emit-2-args.js n=2000000 1.05 % ** 0.001764852
events/ee-emit-3-args.js n=2000000 2.18 % *** 3.234954e-08
events/ee-emit-6-args.js n=2000000 17.17 % *** 1.298702e-103
events/ee-emit-10-args.js n=2000000 17.14 % *** 1.144958e-97
This has a knock-on effect for modules that use events extensively,
such as http2:
http2/headers.js nheaders=0 n=1000 2.10 % *** 6.792106e-11
PR-URL: https://github.com/nodejs/node/pull/16212
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Each conditional branch in EventEmitter.prototype.listeners assigns
its return value to a variable ret which is returned at the end.
Instead just return from within each branch. This is both clearer
and more performant.
events/ee-listeners.js n=5000000 3.65 % *** 3.359171e-10
PR-URL: https://github.com/nodejs/node/pull/16212
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Replace truthy/falsey checks of _events and _events[type] with
comparisons to undefined for better performance:
events/ee-add-remove.js n=250000 5.30 % *** 4.260028e-07
events/ee-emit.js n=2000000 4.18 % *** 1.026649e-05
This has a knock-on effect on modules that use lots of events, e.g.:
http2/headers.js nheaders=0 n=1000 2.60 % *** 0.000298338
PR-URL: https://github.com/nodejs/node/pull/16212
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.
PR-URL: https://github.com/nodejs/node/pull/15473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.
PR-URL: https://github.com/nodejs/node/pull/15281
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.
PR-URL: https://github.com/nodejs/node/pull/15153
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests
Refs: https://github.com/expressjs/express/pull/3390#discussion_r136718729
PR-URL: https://github.com/nodejs/node/pull/15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
[kFinish](code) can be triggered from a 'finish' event (for example
when calling response.end) which does not pass code. That tries to
set closedCode to undefined resulting in NaN closedCode instead of
NGHTTP2_NO_ERROR. Check for code !== undefined before setting.
Adds tests for closed and closedCode.
PR-URL: https://github.com/nodejs/node/pull/15154
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Change error types emitted from request and validatePriorityOptions
to be TypeError rather than the incorrect RangeError. Add tests
to confirm that all errors are thrown as expected (weight, parent,
exclusive, silent, endStream and getTrailers). Add test for method
CONNECT throwing error on missing :authority or superfluous :scheme
and :path.
PR-URL: https://github.com/nodejs/node/pull/15109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adds an implementation for writeContinue based on the h2 spec & the
existing http implementation. ClientHttp2Stream now also emits
a continue event when it receives headers with :status 100.
Includes two test cases for default server continue behaviour and
for the exposed checkContinue listener.
PR-URL: https://github.com/nodejs/node/pull/15039
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Anatoli Papirovski