From 41637a530efb761c713a36f1465cc5ea9b976900 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 15 Aug 2019 15:03:43 +0200
Subject: [PATCH] http2: remove callback-based padding

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: https://github.com/nodejs/node/pull/29144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 doc/api/http2.md                             | 58 +++++---------------
 lib/internal/http2/core.js                   | 19 -------
 src/env.h                                    |  1 -
 src/node_http2.cc                            | 43 +--------------
 src/node_http2.h                             | 10 +---
 src/node_http2_state.h                       | 14 -----
 test/parallel/test-http2-padding-aligned.js  |  5 +-
 test/parallel/test-http2-padding-callback.js | 51 -----------------
 8 files changed, 25 insertions(+), 176 deletions(-)
 delete mode 100644 test/parallel/test-http2-padding-callback.js

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 805a720a79ccee..4dac23b0395a3a 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 69666bb6ba5853..4e633a38e6a4f9 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
 );
diff --git a/src/env.h b/src/env.h
index 9a0f98e121b380..15c2f953133089 100644
--- a/src/env.h
+++ b/src/env.h
@@ -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 38686dc7502fdc..485d54880bd7eb 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 792078d1261dd0..db85dc6e5a8fd2 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 692299a187f60a..1ba3677f47a4f6 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 183eaef7389360..432e3e8629f379 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 6d6a6b27221b07..00000000000000
--- 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();
-}));