Skip to content

Commit

Permalink
http2: remove callback-based padding
Browse files Browse the repository at this point in the history
This option is not useful in practice, as mentioned in comments and the
documentation, because the overhead of calling into JS makes it
unreasonably expensive.

PR-URL: #29144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
addaleax authored and Trott committed Aug 18, 2019
1 parent 5dee17b commit 41637a5
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 176 deletions.
58 changes: 15 additions & 43 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -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()`
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
19 changes: 0 additions & 19 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -3018,7 +3000,6 @@ binding.setCallbackFunctions(
onGoawayData,
onAltSvc,
onOrigin,
onSelectPadding,
onStreamTrailers,
onStreamClose
);
Expand Down
1 change: 0 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
43 changes: 3 additions & 40 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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()); \
Expand All @@ -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
}
Expand All @@ -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());
Expand All @@ -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);
Expand Down
10 changes: 3 additions & 7 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
14 changes: 0 additions & 14 deletions src/node_http2_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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;

Expand All @@ -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];
};
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-http2-padding-aligned.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

{
Expand Down Expand Up @@ -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);
Loading

0 comments on commit 41637a5

Please sign in to comment.