diff options
| -rw-r--r-- | doc/api/http2.md | 58 | ||||
| -rw-r--r-- | lib/internal/http2/core.js | 19 | ||||
| -rw-r--r-- | src/env.h | 1 | ||||
| -rw-r--r-- | src/node_http2.cc | 43 | ||||
| -rw-r--r-- | src/node_http2.h | 10 | ||||
| -rw-r--r-- | src/node_http2_state.h | 14 | ||||
| -rw-r--r-- | test/parallel/test-http2-padding-aligned.js | 5 | ||||
| -rw-r--r-- | test/parallel/test-http2-padding-callback.js | 51 |
8 files changed, 25 insertions, 176 deletions
diff --git a/doc/api/http2.md b/doc/api/http2.md index 805a720a79..4dac23b039 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1929,6 +1929,11 @@ error will be thrown. <!-- YAML added: v8.4.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/29144 + description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to + providing `PADDING_STRATEGY_ALIGNED` and `selectPadding` + has been removed. - version: v12.4.0 pr-url: https://github.com/nodejs/node/pull/27782 description: The `options` parameter now supports `net.createServer()` @@ -1975,9 +1980,6 @@ changes: * `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum amount of padding, as determined by the internal implementation, is to be applied. - * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user - provided `options.selectPadding()` callback is to be used to determine - the amount of padding. * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a @@ -1989,9 +1991,6 @@ changes: streams for the remote peer as if a `SETTINGS` frame had been received. Will be overridden if the remote peer sets its own value for `maxConcurrentStreams`. **Default:** `100`. - * `selectPadding` {Function} When `options.paddingStrategy` is equal to - `http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function - used to determine the padding. See [Using `options.selectPadding()`][]. * `settings` {HTTP/2 Settings Object} The initial settings to send to the remote peer upon connection. * `Http1IncomingMessage` {http.IncomingMessage} Specifies the @@ -2044,6 +2043,11 @@ server.listen(80); <!-- YAML added: v8.4.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/29144 + description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to + providing `PADDING_STRATEGY_ALIGNED` and `selectPadding` + has been removed. - version: v10.12.0 pr-url: https://github.com/nodejs/node/pull/22956 description: Added the `origins` option to automatically send an `ORIGIN` @@ -2090,9 +2094,6 @@ changes: * `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum amount of padding, as determined by the internal implementation, is to be applied. - * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user - provided `options.selectPadding()` callback is to be used to determine - the amount of padding. * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a @@ -2104,9 +2105,6 @@ changes: streams for the remote peer as if a `SETTINGS` frame had been received. Will be overridden if the remote peer sets its own value for `maxConcurrentStreams`. **Default:** `100`. - * `selectPadding` {Function} When `options.paddingStrategy` is equal to - `http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function - used to determine the padding. See [Using `options.selectPadding()`][]. * `settings` {HTTP/2 Settings Object} The initial settings to send to the remote peer upon connection. * ...: Any [`tls.createServer()`][] options can be provided. For @@ -2146,6 +2144,11 @@ server.listen(80); <!-- YAML added: v8.4.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/29144 + description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to + providing `PADDING_STRATEGY_ALIGNED` and `selectPadding` + has been removed. - version: v8.9.3 pr-url: https://github.com/nodejs/node/pull/17105 description: Added the `maxOutstandingPings` option with a default limit of @@ -2191,9 +2194,6 @@ changes: * `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum amount of padding, as determined by the internal implementation, is to be applied. - * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user - provided `options.selectPadding()` callback is to be used to determine - the amount of padding. * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply enough padding to ensure that the total frame length, including the 9-byte header, is a multiple of 8. For each frame, however, there is a @@ -2205,9 +2205,6 @@ changes: streams for the remote peer as if a `SETTINGS` frame had been received. Will be overridden if the remote peer sets its own value for `maxConcurrentStreams`. **Default:** `100`. - * `selectPadding` {Function} When `options.paddingStrategy` is equal to - `http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function - used to determine the padding. See [Using `options.selectPadding()`][]. * `settings` {HTTP/2 Settings Object} The initial settings to send to the remote peer upon connection. * `createConnection` {Function} An optional callback that receives the `URL` @@ -2389,30 +2386,6 @@ properties. All additional properties on the settings object are ignored. -### Using `options.selectPadding()` - -When `options.paddingStrategy` is equal to -`http2.constants.PADDING_STRATEGY_CALLBACK`, the HTTP/2 implementation will -consult the `options.selectPadding()` callback function, if provided, to -determine the specific amount of padding to use per `HEADERS` and `DATA` frame. - -The `options.selectPadding()` function receives two numeric arguments, -`frameLen` and `maxFrameLen` and must return a number `N` such that -`frameLen <= N <= maxFrameLen`. - -```js -const http2 = require('http2'); -const server = http2.createServer({ - paddingStrategy: http2.constants.PADDING_STRATEGY_CALLBACK, - selectPadding(frameLen, maxFrameLen) { - return maxFrameLen; - } -}); -``` - -The `options.selectPadding()` function is invoked once for *every* `HEADERS` and -`DATA` frame. This has a definite noticeable impact on performance. - ### Error Handling There are several types of error conditions that may arise when using the @@ -3498,7 +3471,6 @@ following additional properties: [RFC 8441]: https://tools.ietf.org/html/rfc8441 [Readable Stream]: stream.html#stream_class_stream_readable [Stream]: stream.html#stream_stream -[Using `options.selectPadding()`]: #http2_using_options_selectpadding [`'checkContinue'`]: #http2_event_checkcontinue [`'connect'`]: #http2_event_connect [`'request'`]: #http2_event_request diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 69666bb6ba..4e633a38e6 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -194,10 +194,6 @@ const kType = Symbol('type'); const kWriteGeneric = Symbol('write-generic'); const { - paddingBuffer, - PADDING_BUF_FRAME_LENGTH, - PADDING_BUF_MAX_PAYLOAD_LENGTH, - PADDING_BUF_RETURN_VALUE, kBitfield, kSessionPriorityListenerCount, kSessionFrameErrorListenerCount, @@ -617,20 +613,6 @@ function onGoawayData(code, lastStreamID, buf) { } } -// Returns the padding to use per frame. The selectPadding callback is set -// on the options. It is invoked with two arguments, the frameLen, and the -// maxPayloadLen. The method must return a numeric value within the range -// frameLen <= n <= maxPayloadLen. -function onSelectPadding() { - const session = this[kOwner]; - if (session.destroyed) - return; - const fn = session[kSelectPadding]; - const frameLen = paddingBuffer[PADDING_BUF_FRAME_LENGTH]; - const maxFramePayloadLen = paddingBuffer[PADDING_BUF_MAX_PAYLOAD_LENGTH]; - paddingBuffer[PADDING_BUF_RETURN_VALUE] = fn(frameLen, maxFramePayloadLen); -} - // When a ClientHttp2Session is first created, the socket may not yet be // connected. If request() is called during this time, the actual request // will be deferred until the socket is ready to go. @@ -3018,7 +3000,6 @@ binding.setCallbackFunctions( onGoawayData, onAltSvc, onOrigin, - onSelectPadding, onStreamTrailers, onStreamClose ); @@ -426,7 +426,6 @@ constexpr size_t kFsStatsBufferLength = V(http2session_on_origin_function, v8::Function) \ V(http2session_on_ping_function, v8::Function) \ V(http2session_on_priority_function, v8::Function) \ - V(http2session_on_select_padding_function, v8::Function) \ V(http2session_on_settings_function, v8::Function) \ V(http2session_on_stream_close_function, v8::Function) \ V(http2session_on_stream_trailers_function, v8::Function) \ diff --git a/src/node_http2.cc b/src/node_http2.cc index 38686dc750..485d54880b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -840,32 +840,6 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, return maxPayloadLen; } -// Used as one of the Padding Strategy functions. Uses a callback to JS land -// to determine the amount of padding for the current frame. This option is -// rather more expensive because of the JS boundary cross. It generally should -// not be the preferred option. -ssize_t Http2Session::OnCallbackPadding(size_t frameLen, - size_t maxPayloadLen) { - if (frameLen == 0) return 0; - Debug(this, "using callback to determine padding"); - Isolate* isolate = env()->isolate(); - HandleScope handle_scope(isolate); - Local<Context> context = env()->context(); - Context::Scope context_scope(context); - - AliasedUint32Array& buffer = env()->http2_state()->padding_buffer; - buffer[PADDING_BUF_FRAME_LENGTH] = frameLen; - buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen; - buffer[PADDING_BUF_RETURN_VALUE] = frameLen; - MakeCallback(env()->http2session_on_select_padding_function(), 0, nullptr); - uint32_t retval = buffer[PADDING_BUF_RETURN_VALUE]; - retval = std::min(retval, static_cast<uint32_t>(maxPayloadLen)); - retval = std::max(retval, static_cast<uint32_t>(frameLen)); - Debug(this, "using padding size %d", retval); - return retval; -} - - // Write data received from the i/o stream to the underlying nghttp2_session. // On each call to nghttp2_session_mem_recv, nghttp2 will begin calling the // various callback functions. Each of these will typically result in a call @@ -1251,9 +1225,6 @@ ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, case PADDING_STRATEGY_ALIGNED: padding = session->OnDWordAlignedPadding(padding, maxPayloadLen); break; - case PADDING_STRATEGY_CALLBACK: - padding = session->OnCallbackPadding(padding, maxPayloadLen); - break; } return padding; } @@ -3024,7 +2995,7 @@ void nghttp2_header::MemoryInfo(MemoryTracker* tracker) const { void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) { Environment* env = Environment::GetCurrent(args); - CHECK_EQ(args.Length(), 12); + CHECK_EQ(args.Length(), 11); #define SET_FUNCTION(arg, name) \ CHECK(args[arg]->IsFunction()); \ @@ -3039,9 +3010,8 @@ void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) { SET_FUNCTION(6, goaway_data) SET_FUNCTION(7, altsvc) SET_FUNCTION(8, origin) - SET_FUNCTION(9, select_padding) - SET_FUNCTION(10, stream_trailers) - SET_FUNCTION(11, stream_close) + SET_FUNCTION(9, stream_trailers) + SET_FUNCTION(10, stream_close) #undef SET_FUNCTION } @@ -3062,9 +3032,6 @@ void Initialize(Local<Object> target, FIXED_ONE_BYTE_STRING(isolate, (name)), \ (field)).FromJust() - // Initialize the buffer used for padding callbacks - SET_STATE_TYPEDARRAY( - "paddingBuffer", state->padding_buffer.GetJSArray()); // Initialize the buffer used to store the session state SET_STATE_TYPEDARRAY( "sessionState", state->session_state_buffer.GetJSArray()); @@ -3083,10 +3050,6 @@ void Initialize(Local<Object> target, env->set_http2_state(std::move(state)); - NODE_DEFINE_CONSTANT(target, PADDING_BUF_FRAME_LENGTH); - NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH); - NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE); - NODE_DEFINE_CONSTANT(target, kBitfield); NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount); NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount); diff --git a/src/node_http2.h b/src/node_http2.h index 792078d126..db85dc6e5a 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -321,11 +321,9 @@ enum padding_strategy_type { PADDING_STRATEGY_ALIGNED, // Padding will ensure all data frames are maxFrameSize PADDING_STRATEGY_MAX, - // Padding will be determined via a JS callback. Note that this can be - // expensive because the callback is called once for every DATA and - // HEADERS frame. For performance reasons, this strategy should be - // avoided. - PADDING_STRATEGY_CALLBACK + // Removed and turned into an alias because it is unreasonably expensive for + // very little benefit. + PADDING_STRATEGY_CALLBACK = PADDING_STRATEGY_ALIGNED }; enum session_state_flags { @@ -877,8 +875,6 @@ class Http2Session : public AsyncWrap, public StreamListener { size_t maxPayloadLen); ssize_t OnMaxFrameSizePadding(size_t frameLength, size_t maxPayloadLen); - ssize_t OnCallbackPadding(size_t frameLength, - size_t maxPayloadLen); // Frame Handler int HandleDataFrame(const nghttp2_frame* frame); diff --git a/src/node_http2_state.h b/src/node_http2_state.h index 692299a187..1ba3677f47 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -55,13 +55,6 @@ namespace http2 { IDX_OPTIONS_FLAGS }; - enum Http2PaddingBufferFields { - PADDING_BUF_FRAME_LENGTH, - PADDING_BUF_MAX_PAYLOAD_LENGTH, - PADDING_BUF_RETURN_VALUE, - PADDING_BUF_FIELD_COUNT - }; - enum Http2StreamStatisticsIndex { IDX_STREAM_STATS_ID, IDX_STREAM_STATS_TIMETOFIRSTBYTE, @@ -111,11 +104,6 @@ class Http2State { offsetof(http2_state_internal, session_stats_buffer), IDX_SESSION_STATS_COUNT, root_buffer), - padding_buffer( - isolate, - offsetof(http2_state_internal, padding_buffer), - PADDING_BUF_FIELD_COUNT, - root_buffer), options_buffer( isolate, offsetof(http2_state_internal, options_buffer), @@ -133,7 +121,6 @@ class Http2State { AliasedFloat64Array stream_state_buffer; AliasedFloat64Array stream_stats_buffer; AliasedFloat64Array session_stats_buffer; - AliasedUint32Array padding_buffer; AliasedUint32Array options_buffer; AliasedUint32Array settings_buffer; @@ -144,7 +131,6 @@ class Http2State { double stream_state_buffer[IDX_STREAM_STATE_COUNT]; double stream_stats_buffer[IDX_STREAM_STATS_COUNT]; double session_stats_buffer[IDX_SESSION_STATS_COUNT]; - uint32_t padding_buffer[PADDING_BUF_FIELD_COUNT]; uint32_t options_buffer[IDX_OPTIONS_FLAGS + 1]; uint32_t settings_buffer[IDX_SETTINGS_COUNT + 1]; }; diff --git a/test/parallel/test-http2-padding-aligned.js b/test/parallel/test-http2-padding-aligned.js index 183eaef738..432e3e8629 100644 --- a/test/parallel/test-http2-padding-aligned.js +++ b/test/parallel/test-http2-padding-aligned.js @@ -5,7 +5,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const http2 = require('http2'); -const { PADDING_STRATEGY_ALIGNED } = http2.constants; +const { PADDING_STRATEGY_ALIGNED, PADDING_STRATEGY_CALLBACK } = http2.constants; const makeDuplexPair = require('../common/duplexpair'); { @@ -66,3 +66,6 @@ const makeDuplexPair = require('../common/duplexpair'); })); req.end(); } + +// PADDING_STRATEGY_CALLBACK has been aliased to mean aligned padding. +assert.strictEqual(PADDING_STRATEGY_ALIGNED, PADDING_STRATEGY_CALLBACK); diff --git a/test/parallel/test-http2-padding-callback.js b/test/parallel/test-http2-padding-callback.js deleted file mode 100644 index 6d6a6b2722..0000000000 --- a/test/parallel/test-http2-padding-callback.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) - common.skip('missing crypto'); -const assert = require('assert'); -const h2 = require('http2'); -const { PADDING_STRATEGY_CALLBACK } = h2.constants; - -function selectPadding(frameLen, max) { - assert.strictEqual(typeof frameLen, 'number'); - assert.strictEqual(typeof max, 'number'); - assert(max >= frameLen); - return max; -} - -// selectPadding will be called three times: -// 1. For the client request headers frame -// 2. For the server response headers frame -// 3. For the server response data frame -const options = { - paddingStrategy: PADDING_STRATEGY_CALLBACK, - selectPadding: common.mustCall(selectPadding, 3) -}; - -const server = h2.createServer(options); -server.on('stream', common.mustCall(onStream)); - -function onStream(stream, headers, flags) { - stream.respond({ - 'content-type': 'text/html', - ':status': 200 - }); - stream.end('hello world'); -} - -server.listen(0); - -server.on('listening', common.mustCall(() => { - const client = h2.connect(`http://localhost:${server.address().port}`, - options); - - const req = client.request({ ':path': '/' }); - req.on('response', common.mustCall()); - req.resume(); - req.on('end', common.mustCall(() => { - server.close(); - client.close(); - })); - req.end(); -})); |