http2: set js callbacks once

Make the http2 binding a bit more efficient by setting the callback functions once when the module is loaded rather than for each `Http2Session` and `Http2Stream`. PR-URL: #24063 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Note: Landed with one collaborator approval after PR was open for 18 days
nodejs · Nov 28, 2018 · 81a705637879f1ad306481f73490bb926e7b6c7c · 81a7056
1 parent cd7df56
commit 81a705637879f1ad306481f73490bb926e7b6c7c
Showing with 100 additions and 63 deletions.

+33 −24 lib/internal/http2/core.js

+12 −11 src/env.h

+55 −28 src/node_http2.cc
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -159,6 +159,7 @@ const kProceed = Symbol('proceed');
 const kProtocol = Symbol('protocol');
 const kProxySocket = Symbol('proxy-socket');
 const kRemoteSettings = Symbol('remote-settings');
+const kSelectPadding = Symbol('select-padding');
 const kSentHeaders = Symbol('sent-headers');
 const kSentTrailers = Symbol('sent-trailers');
 const kServer = Symbol('server');
@@ -368,8 +369,8 @@ function onPing(payload) {
 // longer usable so destroy it also.
 function onStreamClose(code) {
   const stream = this[kOwner];
-  if (stream.destroyed)
-    return;
+  if (!stream || stream.destroyed)
+    return false;
 
   debug(`Http2Stream ${stream[kID]} [Http2Session ` +
         `${sessionName(stream[kSession][kType])}]: closed with code ${code}`);
@@ -399,6 +400,7 @@ function onStreamClose(code) {
     else
       stream.read(0);
   }
+  return true;
 }
 
 // Called when the remote peer settings have been updated.
@@ -523,12 +525,14 @@ function onGoawayData(code, lastStreamID, buf) {
 // 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(fn) {
-  return function getPadding() {
-    const frameLen = paddingBuffer[PADDING_BUF_FRAME_LENGTH];
-    const maxFramePayloadLen = paddingBuffer[PADDING_BUF_MAX_PAYLOAD_LENGTH];
-    paddingBuffer[PADDING_BUF_RETURN_VALUE] = fn(frameLen, maxFramePayloadLen);
-  };
+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
@@ -826,28 +830,20 @@ function setupHandle(socket, type, options) {
     process.nextTick(emit, this, 'connect', this, socket);
     return;
   }
+
+  assert(socket._handle !== undefined,
+         'Internal HTTP/2 Failure. The socket is not connected. Please ' +
+         'report this as a bug in Node.js');
+
   debug(`Http2Session ${sessionName(type)}: setting up session handle`);
   this[kState].flags |= SESSION_FLAGS_READY;
 
   updateOptionsBuffer(options);
   const handle = new binding.Http2Session(type);
   handle[kOwner] = this;
-  handle.error = onSessionInternalError;
-  handle.onpriority = onPriority;
-  handle.onsettings = onSettings;
-  handle.onping = onPing;
-  handle.onheaders = onSessionHeaders;
-  handle.onframeerror = onFrameError;
-  handle.ongoawaydata = onGoawayData;
-  handle.onaltsvc = onAltSvc;
-  handle.onorigin = onOrigin;
 
   if (typeof options.selectPadding === 'function')
-    handle.ongetpadding = onSelectPadding(options.selectPadding);
-
-  assert(socket._handle !== undefined,
-         'Internal HTTP/2 Failure. The socket is not connected. Please ' +
-         'report this as a bug in Node.js');
+    this[kSelectPadding] = options.selectPadding;
   handle.consume(socket._handle._externalStream);
 
   this[kHandle] = handle;
@@ -1654,8 +1650,6 @@ class Http2Stream extends Duplex {
     this[async_id_symbol] = handle.getAsyncId();
     handle[kOwner] = this;
     this[kHandle] = handle;
-    handle.ontrailers = onStreamTrailers;
-    handle.onstreamclose = onStreamClose;
     handle.onread = onStreamRead;
     this.uncork();
     this.emit('ready');
@@ -2899,6 +2893,21 @@ function getUnpackedSettings(buf, options = {}) {
   return settings;
 }
 
+binding.setCallbackFunctions(
+  onSessionInternalError,
+  onPriority,
+  onSettings,
+  onPing,
+  onSessionHeaders,
+  onFrameError,
+  onGoawayData,
+  onAltSvc,
+  onOrigin,
+  onSelectPadding,
+  onStreamTrailers,
+  onStreamClose
+);
+
 // Exports
 module.exports = {
   connect,

diff --git a/src/env.h b/src/env.h
@@ -212,7 +212,6 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
   V(nistcurve_string, "nistCurve")                                             \
   V(nsname_string, "nsname")                                                   \
   V(ocsp_request_string, "OCSPRequest")                                        \
-  V(onaltsvc_string, "onaltsvc")                                               \
   V(oncertcb_string, "oncertcb")                                               \
   V(onchange_string, "onchange")                                               \
   V(onclienthello_string, "onclienthello")                                     \
@@ -221,26 +220,16 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
   V(ondone_string, "ondone")                                                   \
   V(onerror_string, "onerror")                                                 \
   V(onexit_string, "onexit")                                                   \
-  V(onframeerror_string, "onframeerror")                                       \
-  V(ongetpadding_string, "ongetpadding")                                       \
-  V(ongoawaydata_string, "ongoawaydata")                                       \
   V(onhandshakedone_string, "onhandshakedone")                                 \
   V(onhandshakestart_string, "onhandshakestart")                               \
-  V(onheaders_string, "onheaders")                                             \
   V(onmessage_string, "onmessage")                                             \
   V(onnewsession_string, "onnewsession")                                       \
   V(onocspresponse_string, "onocspresponse")                                   \
-  V(onorigin_string, "onorigin")                                               \
-  V(onping_string, "onping")                                                   \
-  V(onpriority_string, "onpriority")                                           \
   V(onread_string, "onread")                                                   \
   V(onreadstart_string, "onreadstart")                                         \
   V(onreadstop_string, "onreadstop")                                           \
-  V(onsettings_string, "onsettings")                                           \
   V(onshutdown_string, "onshutdown")                                           \
   V(onsignal_string, "onsignal")                                               \
-  V(onstreamclose_string, "onstreamclose")                                     \
-  V(ontrailers_string, "ontrailers")                                           \
   V(onunpipe_string, "onunpipe")                                               \
   V(onwrite_string, "onwrite")                                                 \
   V(openssl_error_stack, "opensslErrorStack")                                  \
@@ -343,6 +332,18 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
   V(host_import_module_dynamically_callback, v8::Function)                     \
   V(host_initialize_import_meta_object_callback, v8::Function)                 \
   V(http2ping_constructor_template, v8::ObjectTemplate)                        \
+  V(http2session_on_altsvc_function, v8::Function)                             \
+  V(http2session_on_error_function, v8::Function)                              \
+  V(http2session_on_frame_error_function, v8::Function)                        \
+  V(http2session_on_goaway_data_function, v8::Function)                        \
+  V(http2session_on_headers_function, v8::Function)                            \
+  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)                    \
   V(http2settings_constructor_template, v8::ObjectTemplate)                    \
   V(http2stream_constructor_template, v8::ObjectTemplate)                      \
   V(immediate_callback_function, v8::Function)                                 \

diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -851,7 +851,7 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
   buffer[PADDING_BUF_FRAME_LENGTH] = frameLen;
   buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen;
   buffer[PADDING_BUF_RETURN_VALUE] = frameLen;
-  MakeCallback(env()->ongetpadding_string(), 0, nullptr);
+  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));
@@ -1017,7 +1017,7 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
     Local<Context> context = env->context();
     Context::Scope context_scope(context);
     Local<Value> arg = Integer::New(isolate, lib_error_code);
-    session->MakeCallback(env->error_string(), 1, &arg);
+    session->MakeCallback(env->http2session_on_error_function(), 1, &arg);
   }
   return 0;
 }
@@ -1054,7 +1054,9 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
     Integer::New(isolate, frame->hd.type),
     Integer::New(isolate, error_code)
   };
-  session->MakeCallback(env->onframeerror_string(), arraysize(argv), argv);
+  session->MakeCallback(
+      env->http2session_on_frame_error_function(),
+      arraysize(argv), argv);
   return 0;
 }
 
@@ -1085,22 +1087,19 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
     return 0;
 
   stream->Close(code);
+
   // It is possible for the stream close to occur before the stream is
-  // ever passed on to the javascript side. If that happens, skip straight
-  // to destroying the stream. We can check this by looking for the
-  // onstreamclose function. If it exists, then the stream has already
-  // been passed on to javascript.
-  Local<Value> fn =
-      stream->object()->Get(context, env->onstreamclose_string())
-          .ToLocalChecked();
-
-  if (!fn->IsFunction()) {
+  // ever passed on to the javascript side. If that happens, the callback
+  // will return false.
+  Local<Value> arg = Integer::NewFromUnsigned(isolate, code);
+  MaybeLocal<Value> answer =
+    stream->MakeCallback(env->http2session_on_stream_close_function(),
+                          1, &arg);
+  if (answer.IsEmpty() ||
+      !(answer.ToLocalChecked()->BooleanValue(env->context()).FromJust())) {
+    // Skip to destroy
     stream->Destroy();
-    return 0;
   }
-
-  Local<Value> arg = Integer::NewFromUnsigned(isolate, code);
-  stream->MakeCallback(fn.As<Function>(), 1, &arg);
   return 0;
 }
 
@@ -1233,7 +1232,7 @@ int Http2Session::OnNghttpError(nghttp2_session* handle,
     Local<Context> context = env->context();
     Context::Scope context_scope(context);
     Local<Value> arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
-    session->MakeCallback(env->error_string(), 1, &arg);
+    session->MakeCallback(env->http2session_on_error_function(), 1, &arg);
   }
   return 0;
 }
@@ -1317,7 +1316,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
       Integer::New(isolate, stream->headers_category()),
       Integer::New(isolate, frame->hd.flags),
       Array::New(isolate, headers_v.data(), headers_size * 2)};
-  MakeCallback(env()->onheaders_string(), arraysize(args), args);
+  MakeCallback(env()->http2session_on_headers_function(),
+               arraysize(args), args);
 }
 
 
@@ -1343,7 +1343,8 @@ void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
     Integer::New(isolate, spec.weight),
     Boolean::New(isolate, spec.exclusive)
   };
-  MakeCallback(env()->onpriority_string(), arraysize(argv), argv);
+  MakeCallback(env()->http2session_on_priority_function(),
+               arraysize(argv), argv);
 }
 
 
@@ -1383,7 +1384,8 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
                            length).ToLocalChecked();
   }
 
-  MakeCallback(env()->ongoawaydata_string(), arraysize(argv), argv);
+  MakeCallback(env()->http2session_on_goaway_data_function(),
+               arraysize(argv), argv);
 }
 
 // Called by OnFrameReceived when a complete ALTSVC frame has been received.
@@ -1411,7 +1413,8 @@ void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {
                            altsvc->field_value_len).ToLocalChecked(),
   };
 
-  MakeCallback(env()->onaltsvc_string(), arraysize(argv), argv);
+  MakeCallback(env()->http2session_on_altsvc_function(),
+               arraysize(argv), argv);
 }
 
 void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) {
@@ -1436,7 +1439,7 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) {
             .ToLocalChecked();
   }
   Local<Value> holder = Array::New(isolate, origin_v.data(), origin_v.size());
-  MakeCallback(env()->onorigin_string(), 1, &holder);
+  MakeCallback(env()->http2session_on_origin_function(), 1, &holder);
 }
 
 // Called by OnFrameReceived when a complete PING frame has been received.
@@ -1457,7 +1460,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
       // is buggy or malicious, and we're not going to tolerate such
       // nonsense.
       arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
-      MakeCallback(env()->error_string(), 1, &arg);
+      MakeCallback(env()->http2session_on_error_function(), 1, &arg);
       return;
     }
 
@@ -1469,15 +1472,15 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
   arg = Buffer::Copy(env(),
                       reinterpret_cast<const char*>(frame->ping.opaque_data),
                       8).ToLocalChecked();
-  MakeCallback(env()->onping_string(), 1, &arg);
+  MakeCallback(env()->http2session_on_ping_function(), 1, &arg);
 }
 
 // Called by OnFrameReceived when a complete SETTINGS frame has been received.
 void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
   bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
   if (!ack) {
     // This is not a SETTINGS acknowledgement, notify and return
-    MakeCallback(env()->onsettings_string(), 0, nullptr);
+    MakeCallback(env()->http2session_on_settings_function(), 0, nullptr);
     return;
   }
 
@@ -1502,7 +1505,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
   Local<Context> context = env()->context();
   Context::Scope context_scope(context);
   Local<Value> arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
-  MakeCallback(env()->error_string(), 1, &arg);
+  MakeCallback(env()->http2session_on_error_function(), 1, &arg);
 }
 
 // Callback used when data has been written to the stream.
@@ -1827,7 +1830,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   if (UNLIKELY(ret < 0)) {
     Debug(this, "fatal error receiving data: %d", ret);
     Local<Value> arg = Integer::New(isolate, ret);
-    MakeCallback(env()->error_string(), 1, &arg);
+    MakeCallback(env()->http2session_on_error_function(), 1, &arg);
     return;
   }
 
@@ -2032,7 +2035,7 @@ void Http2Stream::OnTrailers() {
   Local<Context> context = env()->context();
   Context::Scope context_scope(context);
   flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
-  MakeCallback(env()->ontrailers_string(), 0, nullptr);
+  MakeCallback(env()->http2session_on_stream_trailers_function(), 0, nullptr);
 }
 
 // Submit informational headers for a stream.
@@ -2898,6 +2901,29 @@ void nghttp2_header::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackFieldWithSize("value", nghttp2_rcbuf_get_buf(value).len);
 }
 
+void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK_EQ(args.Length(), 12);
+
+#define SET_FUNCTION(arg, name)                                               \
+  CHECK(args[arg]->IsFunction());                                             \
+  env->set_http2session_on_ ## name ## _function(args[arg].As<Function>());
+
+  SET_FUNCTION(0, error)
+  SET_FUNCTION(1, priority)
+  SET_FUNCTION(2, settings)
+  SET_FUNCTION(3, ping)
+  SET_FUNCTION(4, headers)
+  SET_FUNCTION(5, frame_error)
+  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)
+
+#undef SET_FUNCTION
+}
 
 // Set up the process.binding('http2') binding.
 void Initialize(Local<Object> target,
@@ -3106,6 +3132,7 @@ HTTP_STATUS_CODES(V)
 
   env->SetMethod(target, "refreshDefaultSettings", RefreshDefaultSettings);
   env->SetMethod(target, "packSettings", PackSettings);
+  env->SetMethod(target, "setCallbackFunctions", SetCallbackFunctions);
 
   target->Set(context,
               env->constants_string(),