Skip to content

Commit

Permalink
http2: set js callbacks once
Browse files Browse the repository at this point in the history 
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: nodejs#24063
Reviewed-By: Matteo Collina <[email protected]>
Note: Landed with one collaborator approval after PR
      was open for 18 days
  • Loading branch information
@jasnell @refack
jasnell authored and refack committed Jan 10, 2019
1 parent 7ab1000 commit 71a09e3
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 63 deletions.
57 changes: 33 additions & 24 deletions lib/internal/http2/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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}`);
Expand Down Expand Up @@ -399,6 +400,7 @@ function onStreamClose(code) {
else
stream.read(0);
}
return true;
}

// Called when the remote peer settings have been updated.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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,
Expand Down
23 changes: 12 additions & 11 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand All @@ -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") \
Expand Down Expand Up @@ -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) \
Expand Down
83 changes: 55 additions & 28 deletions src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

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


Expand All @@ -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);
}


Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -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.
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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.
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down

0 comments on commit 71a09e3

Please sign in to comment.