Skip to content

http2: major update to internals #17105

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

Closed
jasnell wants to merge 2 commits into from
Closed

http2: major update to internals #17105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a21dca0
http2: major update to internals
jasnell Nov 15, 2017
b05f1dc
[Squash] Fix nits
jasnell Nov 21, 2017
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
Prev Previous commit
[Squash] Fix nits
  • Loading branch information
@jasnell
jasnell committed Nov 21, 2017
commit b05f1dccbed989563e7e678ca2c906e610859312
2 changes: 1 addition & 1 deletion doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@ An HTTP/2 ping was cancelled.
<a id="ERR_HTTP2_PING_LENGTH"></a>
### ERR_HTTP2_PING_LENGTH

HTTP/2 ping payloads must be exactly 8-bytes in length.
HTTP/2 ping payloads must be exactly 8 bytes in length.

<a id="ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED"></a>
### ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED
Expand Down
2 changes: 1 addition & 1 deletion doc/api/http2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ The maximum number of outstanding (unacknowledged) pings is determined by the
`maxOutstandingPings` configuration option. The default maximum is 10.

If provided, the `payload` must be a `Buffer`, `TypedArray`, or `DataView`
containing 8-bytes of data that will be transmitted with the `PING` and
containing 8 bytes of data that will be transmitted with the `PING` and
returned with the ping acknowledgement.

The callback will be invoked with three arguments: an error argument that will
Expand Down
9 changes: 1 addition & 8 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
require('internal/util').assertCrypto();

const { async_id_symbol } = process.binding('async_wrap');
const { newUid, setInitTriggerId } = require('async_hooks');
const binding = process.binding('http2');
const assert = require('assert');
const { Buffer } = require('buffer');
Expand Down Expand Up @@ -1132,7 +1131,6 @@ function onHandleFinish() {
const req = new ShutdownWrap();
req.oncomplete = () => {};
req.handle = handle;
setInitTriggerId(this[async_id_symbol]);
handle.shutdown(req);
}
}
Expand Down Expand Up @@ -1188,11 +1186,6 @@ function abort(stream) {
}
}

function getNewAsyncId(handle) {
return (!handle || typeof handle.getAsyncId !== 'function') ?
newUid() : handle.getAsyncId();
}

// An Http2Stream is a Duplex stream that is backed by a
// node::http2::Http2Stream handle implementing StreamBase.
class Http2Stream extends Duplex {
Expand Down Expand Up @@ -1232,7 +1225,7 @@ class Http2Stream extends Duplex {

[kInit](id, handle) {
this[kID] = id;
this[async_id_symbol] = getNewAsyncId(handle);
this[async_id_symbol] = handle.getAsyncId();
handle[kOwner] = this;
this[kHandle] = handle;
handle.ontrailers = onStreamTrailers;
Expand Down
82 changes: 37 additions & 45 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ Http2Session::Http2Session(Environment* env,
max_header_pairs_ =
type == NGHTTP2_SESSION_SERVER
? std::max(maxHeaderPairs, 4) // minimum # of request headers
: std::max(maxHeaderPairs, 1); // minumum # of response headers
: std::max(maxHeaderPairs, 1); // minimum # of response headers

max_outstanding_pings_ = opts.GetMaxOutstandingPings();

Expand All @@ -336,11 +336,7 @@ Http2Session::Http2Session(Environment* env,
nghttp2_session_callbacks* callbacks
= callback_struct_saved[hasGetPaddingCallback ? 1 : 0].callbacks;

typedef int (*init_fn)(nghttp2_session** session,
const nghttp2_session_callbacks* callbacks,
void* user_data,
const nghttp2_option* options);
init_fn fn = type == NGHTTP2_SESSION_SERVER ?
auto fn = type == NGHTTP2_SESSION_SERVER ?
nghttp2_session_server_new2 :
nghttp2_session_client_new2;

Expand Down Expand Up @@ -444,9 +440,8 @@ inline ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
size_t maxPayloadLen) {
DEBUG_HTTP2SESSION(this, "using callback to determine padding");
Isolate* isolate = env()->isolate();
Local<Context> context = env()->context();

HandleScope handle_scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

#if defined(DEBUG) && DEBUG
Expand Down Expand Up @@ -533,9 +528,6 @@ inline int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle,

Http2Stream* stream = session->FindStream(id);
if (stream == nullptr) {
// Creates the stream and adds it automatically to the Http2Session
// TODO(jasnell): This current makes the Http2Stream instance "weak"
// which may be problematic. Verify.
new Http2Stream(session, id, frame->headers.cat);
} else {
stream->StartHeaders(frame->headers.cat);
Expand Down Expand Up @@ -608,9 +600,9 @@ inline int Http2Session::OnFrameNotSent(nghttp2_session* handle,
if (error_code != NGHTTP2_ERR_SESSION_CLOSING &&
error_code != NGHTTP2_ERR_STREAM_CLOSED &&
error_code != NGHTTP2_ERR_STREAM_CLOSING) {
Local<Context> context = env->context();
Isolate* isolate = env->isolate();
HandleScope scope(isolate);
Local<Context> context = env->context();
Context::Scope context_scope(context);

Local<Value> argv[3] = {
Expand All @@ -630,26 +622,23 @@ inline int Http2Session::OnStreamClose(nghttp2_session* handle,
void* user_data) {
Http2Session* session = static_cast<Http2Session*>(user_data);
Environment* env = session->env();
Isolate* isolate = env->isolate();
HandleScope scope(isolate);
Local<Context> context = env->context();
Context::Scope context_scope(context);
DEBUG_HTTP2SESSION2(session, "stream %d closed with code: %d", id, code);
Http2Stream* stream = session->FindStream(id);
// Intentionally ignore the callback if the stream does not exist
if (stream != nullptr) {
stream->Close(code);

Isolate* isolate = env->isolate();
Local<Context> context = env->context();

// It is possible for the stream close to occur before the stream is
// ever passed on to the javascript side. If that happens, ignore this.
if (stream->object()->Has(context,
env->onstreamclose_string()).FromJust()) {
HandleScope scope(isolate);
Context::Scope context_scope(context);

Local<Value> argv[1] = {
Integer::NewFromUnsigned(isolate, code)
};
stream->MakeCallback(env->onstreamclose_string(), arraysize(argv), argv);
Local<Value> fn =
stream->object()->Get(context, env->onstreamclose_string())
.ToLocalChecked();
if (fn->IsFunction()) {
Local<Value> argv[1] = { Integer::NewFromUnsigned(isolate, code) };
stream->MakeCallback(fn.As<Function>(), arraysize(argv), argv);
}
}
return 0;
Expand Down Expand Up @@ -713,9 +702,9 @@ inline int Http2Session::OnNghttpError(nghttp2_session* handle,
DEBUG_HTTP2SESSION2(session, "Error '%.*s'", len, message);
if (strncmp(message, BAD_PEER_MESSAGE, len) == 0) {
Environment* env = session->env();
Local<Context> context = env->context();
Isolate* isolate = env->isolate();
HandleScope scope(isolate);
Copy link
Copy Markdown
Member

@addaleax addaleax Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, the HandleScope should be the very first thing in this block

Local<Context> context = env->context();
Context::Scope context_scope(context);

Local<Value> argv[1] = {
Expand Down Expand Up @@ -755,17 +744,18 @@ inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers,


inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

int32_t id = GetFrameID(frame);
DEBUG_HTTP2SESSION2(this, "handle headers frame for stream %d", id);
Http2Stream* stream = FindStream(id);

nghttp2_header* headers = stream->headers();
size_t count = stream->headers_count();

Local<Context> context = env()->context();
Isolate* isolate = env()->isolate();
Context::Scope context_scope(context);
HandleScope scope(isolate);
Local<String> name_str;
Local<String> value_str;

Expand Down Expand Up @@ -815,17 +805,17 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {


inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

nghttp2_priority priority_frame = frame->priority;
int32_t id = GetFrameID(frame);
DEBUG_HTTP2SESSION2(this, "handle priority frame for stream %d", id);
// Priority frame stream ID should never be <= 0. nghttp2 handles this for us
nghttp2_priority_spec spec = priority_frame.pri_spec;

Local<Context> context = env()->context();
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Context::Scope context_scope(context);

Local<Value> argv[4] = {
Integer::New(isolate, id),
Integer::New(isolate, spec.stream_id),
Expand All @@ -851,14 +841,14 @@ inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) {


inline void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
nghttp2_goaway goaway_frame = frame->goaway;
DEBUG_HTTP2SESSION(this, "handling goaway frame");

Local<Context> context = env()->context();
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Copy link
Copy Markdown
Member

@addaleax addaleax Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Local<Context> context = env()->context();
Context::Scope context_scope(context);

nghttp2_goaway goaway_frame = frame->goaway;
DEBUG_HTTP2SESSION(this, "handling goaway frame");

Local<Value> argv[3] = {
Integer::NewFromUnsigned(isolate, goaway_frame.error_code),
Integer::New(isolate, goaway_frame.last_stream_id),
Expand Down Expand Up @@ -886,9 +876,9 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {


inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
Local<Context> context = env()->context();
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Copy link
Copy Markdown
Member

@addaleax addaleax Nov 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Local<Context> context = env()->context();
Context::Scope context_scope(context);

bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
Expand Down Expand Up @@ -1136,9 +1126,9 @@ nghttp2_stream* Http2Stream::operator*() {

void Http2Stream::OnTrailers(const SubmitTrailers& submit_trailers) {
DEBUG_HTTP2STREAM(this, "prompting for trailers");
Local<Context> context = env()->context();
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

Local<Value> ret =
Expand Down Expand Up @@ -1595,7 +1585,10 @@ void HttpErrorString(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
uint32_t val = args[0]->Uint32Value(env->context()).ToChecked();
args.GetReturnValue().Set(
OneByteString(env->isolate(), nghttp2_strerror(val)));
String::NewFromOneByte(
env->isolate(),
reinterpret_cast<const uint8_t*>(nghttp2_strerror(val)),
v8::NewStringType::kInternalized).ToLocalChecked());
}


Expand Down Expand Up @@ -1799,12 +1792,11 @@ void Http2Session::Goaway(const FunctionCallbackInfo<Value>& args) {


void Http2Session::UpdateChunksSent(const FunctionCallbackInfo<Value>& args) {
Http2Session* session;
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());

HandleScope scope(isolate);
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());

uint32_t length = session->chunks_sent_since_last_write_;

Expand Down
2 changes: 1 addition & 1 deletion src/node_http2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ class Http2Stream::Provider {
public:
Provider(Http2Stream* stream, int options);
explicit Provider(int options);
~Provider();
virtual ~Provider();

nghttp2_data_provider* operator*() {
return !empty_ ? &provider_ : nullptr;
Expand Down