Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http2: cleanup and refactoring of http2 internals #32884

Closed
wants to merge 18 commits into from
Closed

http2: cleanup and refactoring of http2 internals #32884

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f38c9ea
http2: cleanup constants in http2 binding
jasnell Apr 15, 2020
219e339
http2: refactor GetPackedSettings to avoid unnecessary AsyncWrap
jasnell Apr 15, 2020
5499c7c
http2: simplify settings to reduce duplicate code
jasnell Apr 15, 2020
dd06df1
http2: improve style consistency and correctness
jasnell Apr 16, 2020
a2efaa7
http2: make EmitStatistics function private
jasnell Apr 16, 2020
b23cecc
http2: un-nest Http2Settings and Http2Ping
jasnell Apr 16, 2020
681bfe4
http2: refactoring and cleanup of Http2Settings and Http2Ping
jasnell Apr 16, 2020
0e0a3a2
http2: avoid ** syntax for readability
jasnell Apr 16, 2020
e39420f
http2: avoid most uses to ToChecked/ToLocalChecked
jasnell Apr 16, 2020
e8a2f5e
http2: use const references for Http2Priority
jasnell Apr 16, 2020
52fcf89
http2: remove unnecessary GetStream function
jasnell Apr 16, 2020
7aefc8f
http2: refactor Http2Scope to use BaseObjectPtr
jasnell Apr 16, 2020
d9c5a40
http2: move utility function to anonymous namespace
jasnell Apr 16, 2020
6b86554
http2: refactor and simplify Origins
jasnell Apr 16, 2020
4ccd690
http2: use BaseObjectPtr for Http2Streams map
jasnell Apr 16, 2020
fe77e23
http2: move MemoryInfo impl to cc
jasnell Apr 16, 2020
764c71d
http2: fixup lint and style issues
jasnell Apr 16, 2020
1342b24
[Squash] fixup
jasnell Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,8 @@ Http2Priority::Http2Priority(Environment* env,
Local<Value> weight,
Local<Value> exclusive) {
Local<Context> context = env->context();
int32_t parent_ = parent->Int32Value(context).ToChecked();
int32_t weight_ = weight->Int32Value(context).ToChecked();
int32_t parent_ = parent->Int32Value(context).FromMaybe(0);
int32_t weight_ = weight->Int32Value(context).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
bool exclusive_ = exclusive->IsTrue();
Debug(env, DebugCategory::HTTP2STREAM,
"Http2Priority: parent: %d, weight: %d, exclusive: %s\n",
Expand Down Expand Up @@ -992,7 +992,8 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
MaybeLocal<Value> answer =
stream->MakeCallback(env->http2session_on_stream_close_function(),
1, &arg);
if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) {
Local<Value> def = v8::False(env->isolate());
if (answer.IsEmpty() || answer.FromMaybe(def)->IsFalse()) {
jasnell marked this conversation as resolved.
Show resolved Hide resolved
// Skip to destroy
stream->Destroy();
}
Expand Down Expand Up @@ -1285,17 +1286,21 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
nghttp2_goaway goaway_frame = frame->goaway;
Debug(this, "handling goaway frame");

Local<Value> undef = Undefined(isolate);
Local<Value> argv[3] = {
Integer::NewFromUnsigned(isolate, goaway_frame.error_code),
Integer::New(isolate, goaway_frame.last_stream_id),
Undefined(isolate)
undef
};

size_t length = goaway_frame.opaque_data_len;
if (length > 0) {
// If the copy fails for any reason here, we just ignore it.
// The additional goaway data is completely optional and we
// shouldn't fail if we're not able to process it.
jasnell marked this conversation as resolved.
Show resolved Hide resolved
argv[2] = Buffer::Copy(isolate,
reinterpret_cast<char*>(goaway_frame.opaque_data),
length).ToLocalChecked();
length).FromMaybe(undef);
}

MakeCallback(env()->http2session_on_goaway_data_function(),
Expand All @@ -1318,14 +1323,8 @@ void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {

Local<Value> argv[3] = {
Integer::New(isolate, id),
String::NewFromOneByte(isolate,
altsvc->origin,
NewStringType::kNormal,
altsvc->origin_len).ToLocalChecked(),
String::NewFromOneByte(isolate,
altsvc->field_value,
NewStringType::kNormal,
altsvc->field_value_len).ToLocalChecked(),
OneByteString(isolate, altsvc->origin, altsvc->origin_len),
OneByteString(isolate, altsvc->field_value, altsvc->field_value_len)
};

MakeCallback(env()->http2session_on_altsvc_function(),
Expand All @@ -1348,10 +1347,7 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) {

for (size_t i = 0; i < nov; ++i) {
const nghttp2_origin_entry& entry = origin->ov[i];
origin_v[i] =
String::NewFromOneByte(
isolate, entry.origin, NewStringType::kNormal, entry.origin_len)
.ToLocalChecked();
origin_v[i] = OneByteString(isolate, entry.origin, entry.origin_len);
}
Local<Value> holder = Array::New(isolate, origin_v.data(), origin_v.size());
MakeCallback(env()->http2session_on_origin_function(), 1, &holder);
Expand Down Expand Up @@ -1385,9 +1381,13 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {

if (!(js_fields_->bitfield & (1 << kSessionHasPingListeners))) return;
// Notify the session that a ping occurred
arg = Buffer::Copy(env(),
reinterpret_cast<const char*>(frame->ping.opaque_data),
8).ToLocalChecked();
Local<Value> undef = Undefined(env()->isolate());
// There are few reasons why the copy should fail, but if it does,
// we'll just return Undefined rather than a buffer.
arg = Buffer::Copy(
env(),
reinterpret_cast<const char*>(frame->ping.opaque_data),
8).FromMaybe(undef);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
MakeCallback(env()->http2session_on_ping_function(), 1, &arg);
}

Expand Down Expand Up @@ -2332,12 +2332,11 @@ void Http2Stream::DecrementAvailableOutboundLength(size_t amount) {
// back to JS land
void HttpErrorString(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
uint32_t val = args[0]->Uint32Value(env->context()).ToChecked();
uint32_t val = args[0]->Uint32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
args.GetReturnValue().Set(
String::NewFromOneByte(
OneByteString(
env->isolate(),
reinterpret_cast<const uint8_t*>(nghttp2_strerror(val)),
NewStringType::kInternalized).ToLocalChecked());
reinterpret_cast<const uint8_t*>(nghttp2_strerror(val))));
}


Expand All @@ -2362,7 +2361,7 @@ void Http2Session::SetNextStreamID(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
int32_t id = args[0]->Int32Value(env->context()).ToChecked();
int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
if (nghttp2_session_set_next_stream_id(session->session(), id) < 0) {
Debug(session, "failed to set next stream id to %d", id);
return args.GetReturnValue().Set(false);
Expand Down Expand Up @@ -2420,8 +2419,10 @@ void Http2Session::New(const FunctionCallbackInfo<Value>& args) {
Http2State* state = Unwrap<Http2State>(args.Data());
Environment* env = state->env();
CHECK(args.IsConstructCall());
int32_t val = args[0]->Int32Value(env->context()).ToChecked();
SessionType type = static_cast<SessionType>(val);
SessionType type =
static_cast<SessionType>(
args[0]->Int32Value(env->context())
.FromMaybe(NGHTTP2_SESSION_SERVER));
jasnell marked this conversation as resolved.
Show resolved Hide resolved
Http2Session* session = new Http2Session(state, args.This(), type);
session->get_async_id(); // avoid compiler warning
Debug(session, "session created");
Expand All @@ -2444,7 +2445,7 @@ void Http2Session::Destroy(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Context> context = env->context();

uint32_t code = args[0]->Uint32Value(context).ToChecked();
uint32_t code = args[0]->Uint32Value(context).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
session->Close(code, args[1]->IsTrue());
}

Expand All @@ -2456,7 +2457,7 @@ void Http2Session::Request(const FunctionCallbackInfo<Value>& args) {
Environment* env = session->env();

Local<Array> headers = args[0].As<Array>();
int32_t options = args[1]->Int32Value(env->context()).ToChecked();
int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
Http2Priority priority(env, args[2], args[3], args[4]);

Debug(session, "request submitted");
Expand Down Expand Up @@ -2506,8 +2507,8 @@ void Http2Session::Goaway(const FunctionCallbackInfo<Value>& args) {
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());

uint32_t code = args[0]->Uint32Value(context).ToChecked();
int32_t lastStreamID = args[1]->Int32Value(context).ToChecked();
uint32_t code = args[0]->Uint32Value(context).FromMaybe(0);
int32_t lastStreamID = args[1]->Int32Value(context).FromMaybe(-1);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
ArrayBufferViewContents<uint8_t> opaque_data;

if (args[2]->IsArrayBufferView()) {
Expand Down Expand Up @@ -2543,7 +2544,7 @@ void Http2Stream::RstStream(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = env->context();
Http2Stream* stream;
ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder());
uint32_t code = args[0]->Uint32Value(context).ToChecked();
uint32_t code = args[0]->Uint32Value(context).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
Debug(stream, "sending rst_stream with code %d", code);
stream->SubmitRstStream(code);
}
Expand All @@ -2556,7 +2557,7 @@ void Http2Stream::Respond(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder());

Local<Array> headers = args[0].As<Array>();
int32_t options = args[1]->Int32Value(env->context()).ToChecked();
int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

args.GetReturnValue().Set(
stream->SubmitResponse(
Expand Down Expand Up @@ -2611,7 +2612,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&parent, args.Holder());

Local<Array> headers = args[0].As<Array>();
int32_t options = args[1]->Int32Value(env->context()).ToChecked();
int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

Debug(parent, "creating push promise");

Expand Down Expand Up @@ -2706,11 +2707,16 @@ void Http2Session::AltSvc(const FunctionCallbackInfo<Value>& args) {
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());

int32_t id = args[0]->Int32Value(env->context()).ToChecked();
int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

// origin and value are both required to be ASCII, handle them as such.
Local<String> origin_str = args[1]->ToString(env->context()).ToLocalChecked();
Local<String> value_str = args[2]->ToString(env->context()).ToLocalChecked();
Local<String> origin_str =
args[1]->ToString(env->context()).FromMaybe(Local<String>());
Local<String> value_str =
args[2]->ToString(env->context()).FromMaybe(Local<String>());

if (origin_str.IsEmpty() || value_str.IsEmpty())
return;
jasnell marked this conversation as resolved.
Show resolved Hide resolved

size_t origin_len = origin_str->Length();
size_t value_len = value_str->Length();
Expand All @@ -2735,8 +2741,7 @@ void Http2Session::Origin(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());

Local<String> origin_string = args[0].As<String>();
int32_t count = args[1]->Int32Value(context).ToChecked();

int32_t count = args[1]->Int32Value(context).FromMaybe(0);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

Origins origins(env->isolate(),
env->context(),
Expand Down Expand Up @@ -2883,7 +2888,7 @@ void Http2Ping::Done(bool ack, const uint8_t* payload) {
if (payload != nullptr) {
buf = Buffer::Copy(isolate,
reinterpret_cast<const char*>(payload),
8).ToLocalChecked();
8).FromMaybe(buf);
}

Local<Value> argv[] = {
Expand Down