From 81a705637879f1ad306481f73490bb926e7b6c7c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 3 Nov 2018 05:04:04 -0700 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/24063 Reviewed-By: Matteo Collina Note: Landed with one collaborator approval after PR was open for 18 days --- lib/internal/http2/core.js | 57 +++++++++++++++----------- src/env.h | 23 ++++++----- src/node_http2.cc | 83 +++++++++++++++++++++++++------------- 3 files changed, 100 insertions(+), 63 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 336e31831692c0..163ede9e7a9ac2 100644 --- 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 index c99f1d68301b6f..64593a0025c3b6 100644 --- 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 index 20cfcf3745c81b..58ba052f04c3cf 100644 --- 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(maxPayloadLen)); retval = std::max(retval, static_cast(frameLen)); @@ -1017,7 +1017,7 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, Local context = env->context(); Context::Scope context_scope(context); Local 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 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 arg = Integer::NewFromUnsigned(isolate, code); + MaybeLocal 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 arg = Integer::NewFromUnsigned(isolate, code); - stream->MakeCallback(fn.As(), 1, &arg); return 0; } @@ -1233,7 +1232,7 @@ int Http2Session::OnNghttpError(nghttp2_session* handle, Local context = env->context(); Context::Scope context_scope(context); Local 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 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,7 +1472,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { arg = Buffer::Copy(env(), reinterpret_cast(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. @@ -1477,7 +1480,7 @@ 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 = env()->context(); Context::Scope context_scope(context); Local 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 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 = 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& 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()); + + 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 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(),