From 81f5e983e1efb4abc82c74621d6a6b605e5d8da2 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 8 Dec 2016 21:32:06 +0100 Subject: [PATCH 01/13] stream_base: homogenize req_wrap_obj use Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. --- src/stream_base.cc | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/stream_base.cc b/src/stream_base.cc index a12a1efc73..3ed622d7ef 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -82,10 +82,8 @@ void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) { req_wrap_obj }; - if (req_wrap->object()->Has(env->context(), - env->oncomplete_string()).FromJust()) { + if (req_wrap_obj->Has(env->context(), env->oncomplete_string()).FromJust()) req_wrap->MakeCallback(env->oncomplete_string(), arraysize(argv), argv); - } delete req_wrap; } @@ -172,9 +170,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { int err = DoWrite(req_wrap, *bufs, count, nullptr); - req_wrap->object()->Set(env->async(), True(env->isolate())); - req_wrap->object()->Set(env->bytes_string(), - Number::New(env->isolate(), bytes)); + req_wrap_obj->Set(env->async(), True(env->isolate())); + req_wrap_obj->Set(env->bytes_string(), Number::New(env->isolate(), bytes)); const char* msg = Error(); if (msg != nullptr) { req_wrap_obj->Set(env->error_string(), OneByteString(env->isolate(), msg)); @@ -328,7 +325,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { // Reference StreamWrap instance to prevent it from being garbage // collected before `AfterWrite` is called. CHECK_EQ(false, req_wrap->persistent().IsEmpty()); - req_wrap->object()->Set(env->handle_string(), send_handle_obj); + req_wrap_obj->Set(env->handle_string(), send_handle_obj); } err = DoWrite( @@ -338,7 +335,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { reinterpret_cast(send_handle)); } - req_wrap->object()->Set(env->async(), True(env->isolate())); + req_wrap_obj->Set(env->async(), True(env->isolate())); if (err) req_wrap->Dispose(); @@ -383,10 +380,8 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) { wrap->ClearError(); } - if (req_wrap->object()->Has(env->context(), - env->oncomplete_string()).FromJust()) { + if (req_wrap_obj->Has(env->context(), env->oncomplete_string()).FromJust()) req_wrap->MakeCallback(env->oncomplete_string(), arraysize(argv), argv); - } req_wrap->Dispose(); } From 1d827e6b52f2ffc6042ed6cfe6c876eb3888b6c9 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 2 Dec 2016 21:31:25 -0600 Subject: [PATCH 02/13] http2: writev and performance improvements --- lib/internal/http2.js | 117 ++++++++++++++-------------- src/node_http2.cc | 174 +++++++++++++++++++++++++++--------------- src/node_http2.h | 54 ++++++------- 3 files changed, 200 insertions(+), 145 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index f88cd125e4..e427a36ff7 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -61,8 +61,10 @@ const tls = require('tls'); const stream = require('stream'); const FreeList = require('internal/freelist').FreeList; const url = require('url'); +const URL = url.URL; const common = require('_http_common'); const WriteWrap = streamwrap.WriteWrap; +const ShutdownWrap = streamwrap.ShutdownWrap; const Writable = stream.Writable; const Readable = stream.Readable; const Duplex = stream.Duplex; @@ -83,7 +85,6 @@ const kOwner = Symbol('owner'); const kNoBody = Symbol('nobody'); const kRequest = Symbol('request'); const kResponse = Symbol('response'); -const kResume = Symbol('resume'); const kSendDate = Symbol('send-date'); const kServer = Symbol('server'); const kSession = Symbol('session'); @@ -266,9 +267,11 @@ function maybeDestroyStream(stream) { class Http2Stream extends Duplex { constructor(session, id, options) { + options.allowHalfOpen = true; super(options); this[kId] = id; this[kSession] = session; + this.on('finish', onHandleFinish); } get _handle() { @@ -320,7 +323,11 @@ class Http2Stream extends Duplex { if (this._handle) { this._handle.changeStreamPriority(parentId, priority, exclusive); } else { - this.once('handle', this.changeStreamPriority.bind(this, parentId, priority, exclusive)); + this.once('handle', + this.changeStreamPriority.bind(this, + parentId, + priority, + exclusive)); } } @@ -332,14 +339,6 @@ class Http2Stream extends Duplex { } } - resume() { - if (this._handle) { - this._handle.resume(); - } else { - this.once('handle', onHandleResume); - } - } - sendContinue() { if (this._handle) { this._handle.sendContinue(); @@ -352,7 +351,11 @@ class Http2Stream extends Duplex { if (this._handle) { this._handle.sendPriority(parentId, priority, exclusive); } else { - this.once('handle', this.sendPriority.bind(this, parentId, priority, exclusive)); + this.once('handle', + this.sendPriority.bind(this, + parentId, + priority, + exclusive)); } } @@ -434,17 +437,25 @@ class Http2Stream extends Duplex { } } - end(chunk, encoding, callback) { - const state = this._writableState; - if (state.ending || state.finished) { - return; - } - super.end(chunk, encoding, callback); - + _writev(data, cb) { if (this._handle) { - this._handle.finishedWriting(); + unrefTimer(this); + const req = new WriteWrap(); + req.handle = this._handle; + req.callback = cb; + req.oncomplete = afterDoStreamWrite; + req.async = false; + const chunks = new Array(data.length << 1); + for (var i = 0; i < data.length; i++) { + const entry = data[i]; + chunks[i * 2] = entry.chunk; + chunks[i * 2 + 1] = entry.encoding; + } + const err = this._handle.writev(req, chunks); + if (err) + throw util._errnoException(err, 'write', req.error); } else { - this.on('handle', onHandleFinishedWriting); + this.once('handle', onHandleWritev(data, cb)); } } @@ -457,12 +468,16 @@ class Http2Stream extends Duplex { } } -function onHandleReadStart() { - this._handle.readStart(); +function afterShutdown() {} +function onHandleFinish() { + const req = new ShutdownWrap(); + req.oncomplete = afterShutdown; + req.handle = this._handle; + this._handle.shutdown(req); } -function onHandleFinishedWriting() { - this._handle.finishedWriting(); +function onHandleReadStart() { + this._handle.readStart(); } function onHandleWrite(data, encoding, cb) { @@ -471,12 +486,14 @@ function onHandleWrite(data, encoding, cb) { }; } -function onHandleRespond() { - this._handle.respond(); +function onHandleWritev(chunks, cb) { + return function onWriteFinished() { + this._writev(chunks, cb); + }; } -function onHandleResume() { - this._handle.resume(); +function onHandleRespond() { + this._handle.respond(); } class Http2Session extends EventEmitter { @@ -696,9 +713,7 @@ class Http2ServerRequest extends Http2Incoming { } function onHttp2OutgoingPipe() { - if (this[kHeadersSent]) - this[kResume](); - else + if (!this[kHeadersSent]) this[kBeginSend](); } @@ -710,7 +725,6 @@ class Http2Outgoing extends Writable { this[kFinished] = false; this[kHeadersSent] = false; this.on('pipe', onHttp2OutgoingPipe); - this.bufferedCallback = null; } get stream() { @@ -794,8 +808,7 @@ class Http2Outgoing extends Writable { const stream = this[kStream]; if (this.stream) { this[kBeginSend](); - this.bufferedCallback = callback; - stream.write(chunk, encoding, outWriteResume); + stream.write(chunk, encoding, callback); } else { this[kFinished] = true; callback(); @@ -818,20 +831,6 @@ class Http2Outgoing extends Writable { this.stream.respond(Boolean(this[kNoBody])); } } - - [kResume]() { - if (this.stream) { - this.stream.resume(); - } - } -} - -function outWriteResume() { - this[kResume](); - const callback = this.bufferedCallback; - this.bufferedCallback = null; - if (typeof callback === 'function') - callback(); } // Represents an HTTP/2 server response message @@ -902,9 +901,7 @@ class Http2ServerResponse extends Http2Outgoing { createPushResponse() { if (!this.pushSupported) return; - if (this[kHeadersSent]) - this[kResume](); - else + if (!this[kHeadersSent]) this[kBeginSend](); return new Http2PushResponse(this); } @@ -1027,7 +1024,7 @@ function isIllegalConnectionSpecificHeader(name, value) { // the number of expected items up front (it's less // expensive to count than it is to reallocate). function mapToHeaders(map) { - const keys = Object.keys(map) + const keys = Object.keys(map); var size = keys.length; for (var i = 0; i < keys.length; i++) { if (Array.isArray(keys[i])) { @@ -1037,7 +1034,7 @@ function mapToHeaders(map) { const ret = new http2.Http2Headers(size); for (i = 0; i < keys.length; i++) { - const key = keys[i] + const key = keys[i]; const value = map[key]; if (Array.isArray(value) && value.length > 0) { for (var k = 0; k < value.length; k++) { @@ -1105,7 +1102,7 @@ function sessionOnStreamClose(stream, code) { setImmediate(maybeDestroyStream, stream); } -function sessionOnError() { +function sessionOnError(error) { const session = this; const server = session[kServer]; const socket = session[kSocket]; @@ -1123,7 +1120,7 @@ function socketOnTimeout() { if (!server.emit('timeout', this)) { // Session timed out, attempt a graceful exit - session.gracefulTerminate(this.destroy.bind(this)); + server[kSession].gracefulTerminate(this.destroy.bind(this)); } } @@ -1241,12 +1238,17 @@ function sessionOnHeaderComplete(stream, flags, headers, category) { } function connectionListener(socket) { + // Turn off the Nagle's for this socket... + // highly recommended for http/2 implementations + // TODO(jasnell): May want to make this a configuration option? + socket.setNoDelay(); const options = this[kOptions]; // Create the Http2Session instance that is unique to this socket. const session = createServerSession(options, socket); session[kServer] = this; socket[kServer] = this; + this[kSession] = session; session.on('error', sessionOnError); @@ -1560,7 +1562,8 @@ class Http2ClientRequest extends Http2Outgoing { authority += `:${options.port}`; const headers = this[kHeaders] = Object.create(null); - headers[constants.HTTP2_HEADER_SCHEME] = options.protocol.slice(0, options.protocol.length - 1); + headers[constants.HTTP2_HEADER_SCHEME] = + options.protocol.slice(0, options.protocol.length - 1); headers[constants.HTTP2_HEADER_METHOD] = options.method; headers[constants.HTTP2_HEADER_AUTHORITY] = authority; headers[constants.HTTP2_HEADER_PATH] = options.pathname; @@ -1617,7 +1620,7 @@ class Http2ClientRequest extends Http2Outgoing { } } -function addTrailers () { +function addTrailers() { const request = this[kRequest]; if (request[kTrailers]) { // key is coerced on a string on set diff --git a/src/node_http2.cc b/src/node_http2.cc index 3bc6db4edb..ef6dd028ee 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -29,7 +29,6 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::Name; -using v8::Number; using v8::Object; using v8::PropertyCallbackInfo; using v8::String; @@ -391,6 +390,8 @@ void Http2Stream::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) { buf->len = suggested_size; } +void Http2Stream::OnAfterWrite(WriteWrap* w, void* ctx) {} + void Http2Stream::OnReadSelf(ssize_t nread, const uv_buf_t* buf, uv_handle_type pending, @@ -484,15 +485,6 @@ void Http2Stream::SetLocalWindowSize(const FunctionCallbackInfo& args) { void Http2Stream::Resume() {; nghttp2_session_resume_data(**session_, id()); - session_->SendIfNecessary(); -} - -// Tells nghttp2 to resume sending DATA frames for this stream. This -// is a non-op if the Http2Stream instance is detached. -void Http2Stream::ResumeData(const FunctionCallbackInfo& args) { - Http2Stream* stream; - ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - stream->Resume(); } // Send a 100-Continue response. In HTTP/2, a 100-continue is implemented @@ -627,7 +619,22 @@ void Http2Stream::FinishedWriting(const FunctionCallbackInfo& args) { Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); stream->writable_ = false; - stream->Resume(); +} + +int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { + HandleScope scope(req_wrap->env()->isolate()); + Context::Scope context_scope(req_wrap->env()->context()); + // DoShutdown is used only to inform the internals + // that the writable side of the Duplex has ended. + // there still might be data pending in the buffer + // to be pushed down, but once that's over, we're + // ready to send END_STREAM. + writable_ = false; + // Just in case we're paused... + Resume(); + req_wrap->Dispatched(); + req_wrap->Done(0); + return 0; } // Called when data has been written on the Writable side of the Http2Stream @@ -639,28 +646,31 @@ int Http2Stream::DoWrite(WriteWrap* w, size_t count, uv_stream_t* send_handle) { // Buffer the data for the Data Provider - CHECK_EQ(send_handle, nullptr); - // Simply write to the outgoing buffer. The buffer will be - // written out when the data provider callback is invoked. - // If the Http2Stream instance has been detached, then it - // does not do any good to keep storing the data. - // TODO(jasnell): Later could likely make this a CHECK + session()->SendIfNecessary(); + + if (w->object()->Has(FIXED_ONE_BYTE_STRING(env()->isolate(), "ending"))) + writable_ = false; + CHECK_EQ(send_handle, nullptr); + size_t len = 0; for (size_t i = 0; i < count; i++) { // Only attempt to write if the buf is not empty + len += bufs[i].len; if (bufs[i].len > 0) NodeBIO::FromBIO(str_out_)->Write(bufs[i].base, bufs[i].len); } - // Whether detached or not, call dispatch and done. + Resume(); + w->Dispatched(); w->Done(0); return 0; } bool Http2Stream::IsAlive() { - return nghttp2_stream_get_state(**this) != NGHTTP2_STREAM_STATE_CLOSED; + return true; + //return nghttp2_stream_get_state(**this) != NGHTTP2_STREAM_STATE_CLOSED; } bool Http2Stream::IsClosing() { @@ -701,24 +711,25 @@ ssize_t Http2Stream::on_read(nghttp2_session* session, ssize_t amount = bio->Read(reinterpret_cast(buf), length); bool done = false; + if (amount == 0) { if (stream->writable_) return NGHTTP2_ERR_DEFERRED; done = true; - } else if (!stream->writable_ && bio->Length() == 0) { + } else if (!stream->writable_ && BIO_pending(stream->str_out_) == 0) { done = true; } if (done) { *flags |= NGHTTP2_DATA_FLAG_EOF; - if (stream->OutgoingTrailersCount() > 0) { - // If there are any trailing headers they have to be - // queued up to send here. - *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; - nghttp2_submit_trailer(session, - stream->id(), - stream->OutgoingTrailers(), - stream->OutgoingTrailersCount()); - } + if (stream->OutgoingTrailersCount() > 0) { + // If there are any trailing headers they have to be + // queued up to send here. + *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; + nghttp2_submit_trailer(session, + stream->id(), + stream->OutgoingTrailers(), + stream->OutgoingTrailersCount()); + } } return amount; } @@ -798,6 +809,11 @@ void Http2Session::Initialize(Environment* env, Consume(external); } +void SendOnLoop(uv_idle_t* handle) { + SessionIdler* idler = ContainerOf(&SessionIdler::idler_, handle); + idler->session()->SendIfNecessary(); +} + // Capture the stream that will this session will use to send and receive data void Http2Session::Consume(Local external) { CHECK(prev_alloc_cb_.is_empty()); @@ -809,10 +825,29 @@ void Http2Session::Consume(Local external) { prev_read_cb_ = stream->read_cb(); stream->set_alloc_cb({ Http2Session::OnAllocImpl, this }); stream->set_read_cb({ Http2Session::OnReadImpl, this }); + + // Creates an idler handle that will call SendIfNecessary + // on every tick of the event loop to ensure that pending + // data is flushed to the stream + idler_ = new SessionIdler(this); + uv_idle_init(env()->event_loop(), idler_->idler()); + uv_unref(reinterpret_cast(idler_->idler())); + uv_idle_start(idler_->idler(), SendOnLoop); } // Release the captured stream (only if currently captured) void Http2Session::Unconsume() { + // Tear down the idler + if (uv_is_active(reinterpret_cast(idler_->idler()))) + uv_idle_stop(idler_->idler()); + auto OnClose = [](uv_handle_t* handle) { + SessionIdler* idler = + ContainerOf(&SessionIdler::idler_, reinterpret_cast(handle)); + delete idler; + }; + uv_close(reinterpret_cast(idler_->idler()), OnClose); + idler_ = nullptr; + if (prev_alloc_cb_.is_empty()) return; stream_->set_alloc_cb(prev_alloc_cb_); @@ -882,41 +917,60 @@ void Http2Session::OnReadImpl(ssize_t nread, nghttp2_session_mem_recv(**session, reinterpret_cast(buf->base), nread); - // Send any pending frame that exist in nghttp2 queue + // Send any pending frames that exist in nghttp2 queue session->SendIfNecessary(); } -// Called by nghttp2 when there is data to send on this session. -// This is generally trigged by calling the Http2Session::SendIfNecessary -// method but there are other APIs that will trigger it also. The data -// buffer passed in contains the serialized frame data to be sent to -// the stream. -ssize_t Http2Session::send(nghttp2_session* session, - const uint8_t* data, - size_t length, - int flags, - void *user_data) { - Http2Session* session_obj = static_cast(user_data); - CHECK_NE(session_obj, nullptr); - Environment* env = session_obj->env(); - - Local req_wrap_obj = - env->write_wrap_constructor_function() - ->NewInstance(env->context()).ToLocalChecked(); - - auto cb = [](WriteWrap* req, int status) {}; - WriteWrap* write_req = WriteWrap::New(env, req_wrap_obj, nullptr, cb); - - uv_buf_t buf[] { - uv_buf_init(const_cast(reinterpret_cast(data)), length) - }; +int Http2Session::SendIfNecessary() { + // If the underlying stream is not alive, or there is no + // data pending to send, do nothing + if (!stream_->IsAlive() || !nghttp2_session_want_write(**this)) + return 0; - if (session_obj->stream_->DoWrite(write_req, buf, arraysize(buf), nullptr)) { - // Ignore Errors - write_req->Dispose(); + Environment* env = this->env(); + BIO* buf = NodeBIO::New(); + NodeBIO::FromBIO(buf)->AssignEnvironment(env); + for (;;) { + // Grab as much data as is currently pending in the queue + const uint8_t* data = nullptr; + ssize_t len = nghttp2_session_mem_send(session_, &data); + if (len <= 0) + break; + NodeBIO::FromBIO(buf)->Write( + reinterpret_cast(data), + len); + } + while (BIO_pending(buf) != 0) { + HandleScope scope(env->isolate()); + // Loop through as long as there is data in the buffer to send + Local req_wrap_obj = + env->write_wrap_constructor_function() + ->NewInstance(env->context()).ToLocalChecked(); + + auto cb = [](WriteWrap* req, int status) { + req->Dispose(); + }; + WriteWrap* write_req = WriteWrap::New(env, req_wrap_obj, nullptr, cb); + + char* data[kSimultaneousBufferCount]; + size_t size[arraysize(data)]; + size_t count = arraysize(data); + size_t len = NodeBIO::FromBIO(buf)->PeekMultiple(data, size, &count); + CHECK(len != 0 && count != 0); + + uv_buf_t outbuf[arraysize(data)]; + for (size_t i = 0; i < count; i++) + outbuf[i] = uv_buf_init(data[i], size[i]); + + if (stream_->DoWrite(write_req, outbuf, count, nullptr)) { + // Ignore Errors + write_req->Dispose(); + } + NodeBIO::FromBIO(buf)->Read(nullptr, len); } - return length; + + return 0; } // Called whenever an RST_STREAM frame has been received. Results @@ -1343,7 +1397,6 @@ void Initialize(Local target, env->SetProtoMethod(stream, "changeStreamPriority", Http2Stream::ChangeStreamPriority); env->SetProtoMethod(stream, "respond", Http2Stream::Respond); - env->SetProtoMethod(stream, "resume", Http2Stream::ResumeData); env->SetProtoMethod(stream, "sendContinue", Http2Stream::SendContinue); env->SetProtoMethod(stream, "sendPriority", Http2Stream::SendPriority); env->SetProtoMethod(stream, "sendRstStream", Http2Stream::SendRstStream); @@ -1351,8 +1404,7 @@ void Initialize(Local target, env->SetProtoMethod(stream, "addHeader", Http2Stream::AddHeader); env->SetProtoMethod(stream, "addTrailer", Http2Stream::AddTrailer); env->SetProtoMethod(stream, "finishedWriting", Http2Stream::FinishedWriting); - StreamBase::AddMethods(env, stream, StreamBase::kFlagHasWritev | - StreamBase::kFlagNoShutdown); + StreamBase::AddMethods(env, stream, StreamBase::kFlagHasWritev); env->set_http2stream_object( stream->GetFunction()->NewInstance(env->context()).ToLocalChecked()); target->Set(http2StreamClassName, stream->GetFunction()); diff --git a/src/node_http2.h b/src/node_http2.h index 660be30423..23e6ef850a 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -144,6 +144,7 @@ enum http2_data_flags { #define THROW_AND_RETURN_UNLESS_HTTP2STREAM(env, obj) \ THROW_AND_RETURN_UNLESS_(http2stream, "Http2Stream", env, obj); +static const int kSimultaneousBufferCount = 10; enum http2_session_type { SESSION_TYPE_SERVER, @@ -491,7 +492,6 @@ class Http2Headers : public BaseObject { std::vector entries_; }; - class Http2Stream : public AsyncWrap, public StreamBase { public: // Get a stored Http2Stream instance from the nghttp2_session, if one exists @@ -510,6 +510,7 @@ class Http2Stream : public AsyncWrap, public StreamBase { NodeBIO::FromBIO(str_out_)->AssignEnvironment(env); provider_.read_callback = Http2Stream::on_read; provider_.source.ptr = this; + set_after_write_cb({ OnAfterWrite, this }); set_alloc_cb({ OnAllocSelf, this }); set_read_cb({ OnReadSelf, this }); outgoing_headers_.reserve(100); @@ -527,6 +528,7 @@ class Http2Stream : public AsyncWrap, public StreamBase { } void Reset(); + void Initialize(Http2Session* session, int32_t id, nghttp2_headers_category category); @@ -560,6 +562,7 @@ class Http2Stream : public AsyncWrap, public StreamBase { void EmitPendingData(); void ReceiveData(const uint8_t* data, size_t len); + static void OnAfterWrite(WriteWrap* w, void* ctx); static void OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx); static void OnReadSelf(ssize_t nread, const uv_buf_t* buf, uv_handle_type pending, void* ctx); @@ -573,7 +576,6 @@ class Http2Stream : public AsyncWrap, public StreamBase { static void ChangeStreamPriority(const FunctionCallbackInfo& args); static void Respond(const FunctionCallbackInfo& args); - static void ResumeData(const FunctionCallbackInfo& args); static void SendContinue(const FunctionCallbackInfo& args); static void SendPriority(const FunctionCallbackInfo& args); static void SendRstStream(const FunctionCallbackInfo& args); @@ -584,13 +586,7 @@ class Http2Stream : public AsyncWrap, public StreamBase { static void GetState(const FunctionCallbackInfo& args); static void SetLocalWindowSize(const FunctionCallbackInfo& args); - int DoShutdown(ShutdownWrap* req_wrap) override { - HandleScope scope(req_wrap->env()->isolate()); - Context::Scope context_scope(req_wrap->env()->context()); - req_wrap->Dispatched(); - req_wrap->Done(0); - return 0; - } + int DoShutdown(ShutdownWrap* req_wrap) override; // Tell nghttp2 to resume sending DATA frames. If // the Http2Stream instance is detached, this is @@ -751,6 +747,26 @@ class Http2Stream : public AsyncWrap, public StreamBase { static_cast(NULL); }; +class SessionIdler { + public: + SessionIdler(Http2Session* session) : session_(session) {} + + ~SessionIdler() { + session_ = nullptr; + } + + Http2Session* session() { + return session_; + } + + uv_idle_t* idler() { + return &idler_; + } + + uv_idle_t idler_; + private: + Http2Session* session_; +}; class Http2Session : public AsyncWrap { public: @@ -760,7 +776,6 @@ class Http2Session : public AsyncWrap { nghttp2_session_callbacks_new(&cb_); #define SET_SESSION_CALLBACK(callbacks, name) \ nghttp2_session_callbacks_set_##name##_callback(callbacks, name); - SET_SESSION_CALLBACK(cb_, send) SET_SESSION_CALLBACK(cb_, on_frame_recv) SET_SESSION_CALLBACK(cb_, on_stream_close) SET_SESSION_CALLBACK(cb_, on_header) @@ -772,7 +787,6 @@ class Http2Session : public AsyncWrap { ~Http2Session() override { nghttp2_session_callbacks_del(cb_); - Reset(); } void Initialize(Environment* env, @@ -782,13 +796,7 @@ class Http2Session : public AsyncWrap { void Unconsume(); void Consume(Local external); void Reset(); - - // Native API - int SendIfNecessary() { - if (nghttp2_session_want_write(session_)) - return nghttp2_session_send(session_); - return 0; - } + int SendIfNecessary(); size_t self_size() const override { return sizeof(*this); @@ -825,10 +833,6 @@ class Http2Session : public AsyncWrap { static void OnReadImpl(ssize_t nread, const uv_buf_t* buf, uv_handle_type pending, void* ctx); - // Called by nghttp2 when there is data to be sent. - static ssize_t send(nghttp2_session* session, const uint8_t* data, - size_t length, int flags, void *user_data); - // Called by nghttp2 when an RST_STREAM frame has been received static int on_rst_stream_frame(Http2Session* session, int32_t id, const nghttp2_frame_hd hd, @@ -883,11 +887,6 @@ class Http2Session : public AsyncWrap { int32_t stream_id, const uint8_t* data, size_t len, void* user_data); - // Called by nghttp2 whenever a frame has been sent. - static int on_frame_send(nghttp2_session* session, - const nghttp2_frame* frame, - void* user_data); - // Called by nghttp2 to select the padding len for any // given frame. static ssize_t select_padding(nghttp2_session *session, @@ -903,6 +902,7 @@ class Http2Session : public AsyncWrap { DoEmitErrorIfFail(this, rv); } + SessionIdler* idler_; enum http2_session_type type_; nghttp2_session* session_; nghttp2_session_callbacks* cb_; From 6bcd5b42330ade14204887686698cda564baab6d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 09:57:42 -0800 Subject: [PATCH 03/13] Remove uses of _handle accessor --- lib/internal/http2.js | 118 +++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 59 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index e427a36ff7..a4f486d3f9 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -194,7 +194,7 @@ function onStreamClose(id, code) { if (stream) { this[kOwner].emit('streamClose', stream, code); this[kOwner][kStreams].delete(id); - freeStream(stream._handle); + freeStream(stream[kHandle]); } } function onError(error) { @@ -260,7 +260,7 @@ function maybeDestroyStream(stream) { !stream._writableState.length)) { timers.unenroll(stream); stream.session[kStreams].delete(stream.id); - freeStream(stream._handle); + freeStream(stream[kHandle]); } } @@ -288,8 +288,8 @@ class Http2Stream extends Duplex { } get uid() { - if (this._handle) - return this._handle.getUid(); + if (this[kHandle]) + return this[kHandle].getUid(); } set id(id) { @@ -306,22 +306,22 @@ class Http2Stream extends Duplex { get state() { const obj = {}; - if (this._handle) - this._handle.getState(obj); + if (this[kHandle]) + this[kHandle].getState(obj); return obj; } setLocalWindowSize(size) { - if (this._handle) { - this._handle.setLocalWindowSize(size); + if (this[kHandle]) { + this[kHandle].setLocalWindowSize(size); } else { this.once('handle', this.setLocalWindowSize.bind(this, size)); } } changeStreamPriority(parentId, priority, exclusive) { - if (this._handle) { - this._handle.changeStreamPriority(parentId, priority, exclusive); + if (this[kHandle]) { + this[kHandle].changeStreamPriority(parentId, priority, exclusive); } else { this.once('handle', this.changeStreamPriority.bind(this, @@ -332,24 +332,24 @@ class Http2Stream extends Duplex { } respond() { - if (this._handle) { - this._handle.respond(); + if (this[kHandle]) { + this[kHandle].respond(); } else { this.once('handle', onHandleRespond); } } sendContinue() { - if (this._handle) { - this._handle.sendContinue(); + if (this[kHandle]) { + this[kHandle].sendContinue(); } else { this.once('handle', this.sendContinue.bind(this)); } } sendPriority(parentId, priority, exclusive) { - if (this._handle) { - this._handle.sendPriority(parentId, priority, exclusive); + if (this[kHandle]) { + this[kHandle].sendPriority(parentId, priority, exclusive); } else { this.once('handle', this.sendPriority.bind(this, @@ -360,32 +360,32 @@ class Http2Stream extends Duplex { } sendRstStream(code) { - if (this._handle) { - this._handle.sendRstStream(code); + if (this[kHandle]) { + this[kHandle].sendRstStream(code); } else { this.once('handle', this.sendRstStream.bind(this, code)); } } sendPushPromise(headers) { - if (this._handle) { - return this._handle.sendPushPromise(mapToHeaders(headers)); + if (this[kHandle]) { + return this[kHandle].sendPushPromise(mapToHeaders(headers)); } else { this.once('handle', this.sendPushPromise.bind(this, headers)); } } addHeader(name, value, noindex) { - if (this._handle) { - this._handle.addHeader(name, value, noindex); + if (this[kHandle]) { + this[kHandle].addHeader(name, value, noindex); } else { this.once('handle', this.addHeader.bind(this, name, value, noindex)); } } addTrailer(name, value, noindex) { - if (this._handle) { - this._handle.addTrailer(name, value, noindex); + if (this[kHandle]) { + this[kHandle].addTrailer(name, value, noindex); } else { this.once('handle', this.addTrailer.bind(this, name, value, noindex)); } @@ -420,15 +420,15 @@ class Http2Stream extends Duplex { } _write(data, encoding, cb) { - if (this._handle) { + if (this[kHandle]) { unrefTimer(this); const req = new WriteWrap(); - req.handle = this._handle; + req.handle = this[kHandle]; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; const enc = data instanceof Buffer ? 'buffer' : encoding; - const err = createWriteReq(req, this._handle, data, enc); + const err = createWriteReq(req, this[kHandle], data, enc); if (err) throw util._errnoException(err, 'write', req.error); this._bytesDispatched += req.bytes; @@ -438,10 +438,10 @@ class Http2Stream extends Duplex { } _writev(data, cb) { - if (this._handle) { + if (this[kHandle]) { unrefTimer(this); const req = new WriteWrap(); - req.handle = this._handle; + req.handle = this[kHandle]; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; @@ -451,7 +451,7 @@ class Http2Stream extends Duplex { chunks[i * 2] = entry.chunk; chunks[i * 2 + 1] = entry.encoding; } - const err = this._handle.writev(req, chunks); + const err = this[kHandle].writev(req, chunks); if (err) throw util._errnoException(err, 'write', req.error); } else { @@ -460,8 +460,8 @@ class Http2Stream extends Duplex { } _read(n) { - if (this._handle) { - this._handle.readStart(); + if (this[kHandle]) { + this[kHandle].readStart(); } else { this.once('handle', onHandleReadStart); } @@ -472,12 +472,12 @@ function afterShutdown() {} function onHandleFinish() { const req = new ShutdownWrap(); req.oncomplete = afterShutdown; - req.handle = this._handle; - this._handle.shutdown(req); + req.handle = this[kHandle]; + this[kHandle].shutdown(req); } function onHandleReadStart() { - this._handle.readStart(); + this[kHandle].readStart(); } function onHandleWrite(data, encoding, cb) { @@ -493,7 +493,7 @@ function onHandleWritev(chunks, cb) { } function onHandleRespond() { - this._handle.respond(); + this[kHandle].respond(); } class Http2Session extends EventEmitter { @@ -508,8 +508,8 @@ class Http2Session extends EventEmitter { } reset() { - if (this._handle) - this._handle.reset(); + if (this[kHandle]) + this[kHandle].reset(); } get _handle() { @@ -517,8 +517,8 @@ class Http2Session extends EventEmitter { } get uid() { - if (this._handle) - return this._handle.getUid(); + if (this[kHandle]) + return this[kHandle].getUid(); } get type() { @@ -527,36 +527,36 @@ class Http2Session extends EventEmitter { get state() { const obj = {}; - if (this._handle) - this._handle.getState(obj); + if (this[kHandle]) + this[kHandle].getState(obj); return obj; } setNextStreamID(id) { - if (this._handle) - this._handle.setNextStreamID(id); + if (this[kHandle]) + this[kHandle].setNextStreamID(id); } setLocalWindowSize(size) { - if (this._handle) - this._handle.setLocalWindowSize(size); + if (this[kHandle]) + this[kHandle].setLocalWindowSize(size); } get remoteSettings() { - if (this._handle) - return this._handle.getRemoteSettings(); + if (this[kHandle]) + return this[kHandle].getRemoteSettings(); } get localSettings() { - if (this._handle) - return this._handle.getLocalSettings(); + if (this[kHandle]) + return this[kHandle].getLocalSettings(); } set localSettings(settings) { if (!(settings instanceof http2.Http2Settings)) throw new TypeError('settings must be an instance of Http2Settings'); - if (this._handle) { - this._handle.setLocalSettings(settings); + if (this[kHandle]) { + this[kHandle].setLocalSettings(settings); } } @@ -575,16 +575,16 @@ class Http2Session extends EventEmitter { if (typeof callback !== 'function') throw new TypeError('callback must be a function'); - this._handle.startGracefulTerminate(); + this[kHandle].startGracefulTerminate(); process.nextTick(() => { - this._handle.terminate(code || constants.NGHTTP2_NO_ERROR); + this[kHandle].terminate(code || constants.NGHTTP2_NO_ERROR); callback(); }); } request(headers, nobody) { - if (this._handle) - return this._handle.request(headers, nobody); + if (this[kHandle]) + return this[kHandle].request(headers, nobody); } } @@ -650,7 +650,7 @@ class Http2Incoming extends Readable { this[kStream] = stream; this[kHeaders] = headers; this[kFinished] = false; - this[kStream]._handle.onread = incomingOnRead; + this[kStream][kHandle].onread = incomingOnRead; } get finished() { @@ -685,7 +685,7 @@ class Http2Incoming extends Readable { } _read(n) { - this[kStream]._handle.readStart(); + this[kStream][kHandle].readStart(); } } @@ -1268,7 +1268,7 @@ function connectionListener(socket) { socket.destroy = function(error) { session.removeAllListeners(); socket.removeAllListeners(); - freeSession(session._handle); + freeSession(session[kHandle]); socket.destroy = destroySocket; destroySocket.call(socket, error); socket[kServer] = undefined; From 608c0547d5be39ce4a82db46078a5e96d08c3fd1 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 10:26:45 -0800 Subject: [PATCH 04/13] Refactor [kBeginSend]() call --- lib/internal/http2.js | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index a4f486d3f9..7cfb3e16ae 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -713,8 +713,10 @@ class Http2ServerRequest extends Http2Incoming { } function onHttp2OutgoingPipe() { - if (!this[kHeadersSent]) - this[kBeginSend](); + if (!this[kHeadersSent]) { + const beginSend = this[kBeginSend]; + beginSend.call(this); + } } // Represents an outbound HTTP message. @@ -806,8 +808,9 @@ class Http2Outgoing extends Writable { if (typeof chunk === 'string') chunk = Buffer.from(chunk, encoding); const stream = this[kStream]; + const beginSend = this[kBeginSend]; if (this.stream) { - this[kBeginSend](); + beginSend.call(this); stream.write(chunk, encoding, callback); } else { this[kFinished] = true; @@ -816,8 +819,9 @@ class Http2Outgoing extends Writable { } end(data, encoding, callback) { + const beginSend = this[kBeginSend]; if (this.stream) { - this[kBeginSend](); + beginSend.call(this); this[kFinished] = true; this.stream.end(data, encoding, callback); return; @@ -899,10 +903,12 @@ class Http2ServerResponse extends Http2Outgoing { } createPushResponse() { + const beginSend = this[kBeginSend]; if (!this.pushSupported) return; - if (!this[kHeadersSent]) - this[kBeginSend](); + if (!this[kHeadersSent]) { + beginSend.call(this); + } return new Http2PushResponse(this); } @@ -912,7 +918,8 @@ class Http2ServerResponse extends Http2Outgoing { if (this.sendDate) this.setHeader('date', utcDate()); } - super[kBeginSend](); + const beginSend = super[kBeginSend]; + beginSend.call(this); } } From c8f5b7ab264c9e1eaf80a32406b8f610dbaa5857 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 11:25:05 -0800 Subject: [PATCH 05/13] Refactor headers --- lib/internal/http2.js | 47 ++++++++++++++++++++----------------------- src/node_http2.cc | 17 ++++++++++++++++ 2 files changed, 39 insertions(+), 25 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index 7cfb3e16ae..6e5a77a1f7 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -79,6 +79,7 @@ const kHandle = Symbol('handle'); const kHeaders = Symbol('headers'); const kHeadersSent = Symbol('headers-sent'); const kId = Symbol('id'); +const kImplicitHeaders = Symbol('implicit-headers'); const kInFlight = Symbol('in-flight'); const kOptions = Symbol('options'); const kOwner = Symbol('owner'); @@ -331,11 +332,11 @@ class Http2Stream extends Duplex { } } - respond() { + respond(nobody, headers) { if (this[kHandle]) { - this[kHandle].respond(); + this[kHandle].respond(nobody, headers); } else { - this.once('handle', onHandleRespond); + this.once('handle', onHandleRespond(nobody, headers)); } } @@ -375,14 +376,6 @@ class Http2Stream extends Duplex { } } - addHeader(name, value, noindex) { - if (this[kHandle]) { - this[kHandle].addHeader(name, value, noindex); - } else { - this.once('handle', this.addHeader.bind(this, name, value, noindex)); - } - } - addTrailer(name, value, noindex) { if (this[kHandle]) { this[kHandle].addTrailer(name, value, noindex); @@ -492,8 +485,10 @@ function onHandleWritev(chunks, cb) { }; } -function onHandleRespond() { - this[kHandle].respond(); +function onHandleRespond(nobody, headers) { + return function(nobody, headers) { + this[kHandle].respond(); + }; } class Http2Session extends EventEmitter { @@ -725,6 +720,7 @@ class Http2Outgoing extends Writable { super(initHttp2OutgoingOptions(options)); this[kStream] = stream; this[kFinished] = false; + this[kHeaders] = []; this[kHeadersSent] = false; this.on('pipe', onHttp2OutgoingPipe); } @@ -755,8 +751,7 @@ class Http2Outgoing extends Writable { if (value === undefined || value === null) { throw new TypeError('Value must not be undefined or null'); } - const stream = this[kStream]; - stream.addHeader(name, value, Boolean(noindex)); + this[kHeaders].push([name, value, Boolean(noindex)]); return this; } @@ -831,8 +826,12 @@ class Http2Outgoing extends Writable { [kBeginSend]() { if (!this[kHeadersSent]) { + const implicitHeaders = this[kImplicitHeaders]; + if (typeof implicitHeaders === 'function') { + implicitHeaders.call(this); + } this[kHeadersSent] = true; - this.stream.respond(Boolean(this[kNoBody])); + this.stream.respond(Boolean(this[kNoBody]), this[kHeaders]); } } } @@ -912,14 +911,11 @@ class Http2ServerResponse extends Http2Outgoing { return new Http2PushResponse(this); } - [kBeginSend]() { - if (!this[kHeadersSent]) { - this.stream.addHeader(constants.HTTP2_HEADER_STATUS, this.statusCode); - if (this.sendDate) - this.setHeader('date', utcDate()); - } - const beginSend = super[kBeginSend]; - beginSend.call(this); + [kImplicitHeaders]() { + // Set implicit headers.. will be called by [kBeginSend]() + if (this.sendDate) + this[kHeaders].unshift(['date', utcDate()]); + this[kHeaders].unshift([constants.HTTP2_HEADER_STATUS, this.statusCode]); } } @@ -1613,10 +1609,11 @@ class Http2ClientRequest extends Http2Outgoing { if (!this[kHeadersSent]) { this[kHeadersSent] = true; const stream = this[kStream]; + this[kImplicitHeaders](); const _handle = stream.session.request(mapToHeaders(this[kHeaders]), true); if (_handle instanceof http2.Http2Stream) { this[kId] = _handle.getId(); - stream.once('handle', addTrailers) + stream.once('handle', addTrailers); stream._handle = _handle; } } diff --git a/src/node_http2.cc b/src/node_http2.cc index ef6dd028ee..3c79dd278c 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -18,6 +18,7 @@ namespace node { +using v8::Array; using v8::Context; using v8::External; using v8::Function; @@ -517,8 +518,24 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); Http2Session* session = stream->session(); + Isolate* isolate = stream->env()->isolate(); CHECK(**session); bool nodata = args[0]->BooleanValue(); + + if (args[1]->IsArray()) { + Local headers = args[1].As(); + for (size_t n = 0; n < headers->Length(); n++) { + Local item = headers->Get(n); + if (item->IsArray()) { + Local header = item.As(); + Utf8Value key(isolate, header->Get(0)); + Utf8Value value(isolate, header->Get(1)); + bool noindex = header->Get(2)->BooleanValue(); + stream->AddHeader(*key, *value, key.length(), value.length(), noindex); + } + } + } + nghttp2_data_provider* provider = nodata ? nullptr : stream->provider(); int rv = nghttp2_submit_response(**session, stream->id(), From c3bbda5368e2fbbb8083e14169ff6f3c0ea60a91 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 11:37:19 -0800 Subject: [PATCH 06/13] Improve handling of responses with no body --- lib/internal/http2.js | 42 +++++++++++++++--------------------------- src/node_http2.cc | 8 ++++---- 2 files changed, 19 insertions(+), 31 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index 6e5a77a1f7..82b87f73a6 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -83,7 +83,6 @@ const kImplicitHeaders = Symbol('implicit-headers'); const kInFlight = Symbol('in-flight'); const kOptions = Symbol('options'); const kOwner = Symbol('owner'); -const kNoBody = Symbol('nobody'); const kRequest = Symbol('request'); const kResponse = Symbol('response'); const kSendDate = Symbol('send-date'); @@ -332,11 +331,11 @@ class Http2Stream extends Duplex { } } - respond(nobody, headers) { + respond(headers) { if (this[kHandle]) { - this[kHandle].respond(nobody, headers); + this[kHandle].respond(headers); } else { - this.once('handle', onHandleRespond(nobody, headers)); + this.once('handle', onHandleRespond(headers)); } } @@ -485,8 +484,8 @@ function onHandleWritev(chunks, cb) { }; } -function onHandleRespond(nobody, headers) { - return function(nobody, headers) { +function onHandleRespond(headers) { + return function(headers) { this[kHandle].respond(); }; } @@ -577,9 +576,9 @@ class Http2Session extends EventEmitter { }); } - request(headers, nobody) { + request(headers) { if (this[kHandle]) - return this[kHandle].request(headers, nobody); + return this[kHandle].request(headers); } } @@ -723,6 +722,12 @@ class Http2Outgoing extends Writable { this[kHeaders] = []; this[kHeadersSent] = false; this.on('pipe', onHttp2OutgoingPipe); + this.on('finish', () => { + this[kFinished] = true; + this[kStream].end(); + const beginSend = this[kBeginSend]; + beginSend.call(this); + }); } get stream() { @@ -813,17 +818,6 @@ class Http2Outgoing extends Writable { } } - end(data, encoding, callback) { - const beginSend = this[kBeginSend]; - if (this.stream) { - beginSend.call(this); - this[kFinished] = true; - this.stream.end(data, encoding, callback); - return; - } - throw new Error('write after end'); - } - [kBeginSend]() { if (!this[kHeadersSent]) { const implicitHeaders = this[kImplicitHeaders]; @@ -831,7 +825,7 @@ class Http2Outgoing extends Writable { implicitHeaders.call(this); } this[kHeadersSent] = true; - this.stream.respond(Boolean(this[kNoBody]), this[kHeaders]); + this.stream.respond(this[kHeaders]); } } } @@ -885,19 +879,13 @@ class Http2ServerResponse extends Http2Outgoing { // the HTTP/2 frames by only sending the response HEADERS frame and // no DATA frames. Otherwise, the current API would result in have to // send at least one possibly empty DATA frame every time... - // - // Note: the nobody arg is a temporary way of handling no-body responses. - // I will be refactoring this API but I wanted a way to experiment with - // the approach a bit. - writeHead(statusCode, headers, nobody) { + writeHead(statusCode, headers) { if (typeof statusCode === 'object') { headers = statusCode; statusCode = constants.HTTP_STATUS_OK; } this.statusCode = statusCode || constants.HTTP_STATUS_OK; this.addHeaders(headers); - if (nobody) - this[kNoBody] = true; return this; } diff --git a/src/node_http2.cc b/src/node_http2.cc index 3c79dd278c..9261ec1827 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -520,10 +520,9 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { Http2Session* session = stream->session(); Isolate* isolate = stream->env()->isolate(); CHECK(**session); - bool nodata = args[0]->BooleanValue(); - if (args[1]->IsArray()) { - Local headers = args[1].As(); + if (args[0]->IsArray()) { + Local headers = args[0].As(); for (size_t n = 0; n < headers->Length(); n++) { Local item = headers->Get(n); if (item->IsArray()) { @@ -536,7 +535,8 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { } } - nghttp2_data_provider* provider = nodata ? nullptr : stream->provider(); + nghttp2_data_provider* provider = + !stream->writable_ ? nullptr : stream->provider(); int rv = nghttp2_submit_response(**session, stream->id(), stream->OutgoingHeaders(), From e4b0fa782370fd79680d71c19cc4fc3549fae099 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 11:55:12 -0800 Subject: [PATCH 07/13] Some cleanup --- src/node_http2.cc | 19 ++----------------- src/node_http2.h | 6 ++++-- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 9261ec1827..ce4b38dab2 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -510,10 +510,8 @@ void Http2Stream::SendContinue(const FunctionCallbackInfo& args) { session->EmitErrorIfFail(rv); } -// Initiate sending a response. Response Headers must have been set -// before calling. This will result in sending an initial HEADERS -// frame (or multiple), zero or more DATA frames, and zero or more -// trailing HEADERS frames. +// Initiate sending a response. First argument must be an array of header +// entries, each entry is an array. e.g. [['foo', 'bar'], ['abc', 'xyx', true]] void Http2Stream::Respond(const FunctionCallbackInfo& args) { Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); @@ -751,18 +749,6 @@ ssize_t Http2Stream::on_read(nghttp2_session* session, return amount; } -// Adds an outgoing header. These must be set before the Http2Stream::Respond -// method is called. Any headers added after that call will not be sent. -void Http2Stream::AddHeader(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Http2Stream* stream; - ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - Utf8Value key(env->isolate(), args[0]); - Utf8Value value(env->isolate(), args[1]); - bool noindex = args[2]->BooleanValue(); - stream->AddHeader(*key, *value, key.length(), value.length(), noindex); -} - // Adds an outgoing trailer. These must be set before the writable side // of the Http2Stream Duplex is end()'ed. Any headers added after that // call will not be sent. Specifically, the trailers vector is processed @@ -1418,7 +1404,6 @@ void Initialize(Local target, env->SetProtoMethod(stream, "sendPriority", Http2Stream::SendPriority); env->SetProtoMethod(stream, "sendRstStream", Http2Stream::SendRstStream); env->SetProtoMethod(stream, "sendPushPromise", Http2Stream::SendPushPromise); - env->SetProtoMethod(stream, "addHeader", Http2Stream::AddHeader); env->SetProtoMethod(stream, "addTrailer", Http2Stream::AddTrailer); env->SetProtoMethod(stream, "finishedWriting", Http2Stream::FinishedWriting); StreamBase::AddMethods(env, stream, StreamBase::kFlagHasWritev); diff --git a/src/node_http2.h b/src/node_http2.h index 23e6ef850a..d43d67dcc4 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -580,7 +580,6 @@ class Http2Stream : public AsyncWrap, public StreamBase { static void SendPriority(const FunctionCallbackInfo& args); static void SendRstStream(const FunctionCallbackInfo& args); static void SendPushPromise(const FunctionCallbackInfo& args); - static void AddHeader(const FunctionCallbackInfo& args); static void AddTrailer(const FunctionCallbackInfo& args); static void GetId(const FunctionCallbackInfo& args); static void GetState(const FunctionCallbackInfo& args); @@ -688,7 +687,10 @@ class Http2Stream : public AsyncWrap, public StreamBase { return &provider_; } - // Adds an *Outgoing* Header. + // Adds an *Outgoing* Header. Ensures that http/2 pseudo headers appear + // properly at the front of the list. Does not filter out duplicates + // TODO(@jasnell): For headers that we know should only be set once, + // we should provide some kind of duplication check void AddHeader(const char* name, const char* value, size_t nlen, size_t vlen, bool noindex = false) { const Http2Header* header = From a81e16a2682fd0c36f5fd2eb22aa4d9ed0e81ce6 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 13:03:28 -0800 Subject: [PATCH 08/13] Avoid deoptimizations --- lib/internal/http2.js | 257 ++++++++++++++++++++---------------------- 1 file changed, 123 insertions(+), 134 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index 82b87f73a6..f8d5040459 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -108,7 +108,7 @@ Object.defineProperty(exports, 'constants', { const sessions = new FreeList('session', 1000, initSessionHandle); function initSessionHandle() { - const session = new http2.Http2Session(); + var session = new http2.Http2Session(); session.onRstStream = onRstStream; session.onGoaway = onGoaway; session.onHeaders = onHeaders; @@ -120,7 +120,8 @@ function initSessionHandle() { function freeSession(session) { if (session) { session.reset(); - session[kOwner][kHandle] = undefined; + var owner = session[kOwner]; + owner[kHandle] = undefined; session[kOwner] = undefined; if (sessions.free(session) === false) session.close(); @@ -130,8 +131,9 @@ function freeSession(session) { function freeStream(stream) { if (stream) { stream.reset(); - stream[kOwner].end(); - stream[kOwner][kHandle] = undefined; + var owner = stream[kOwner]; + owner.end(); + owner[kHandle] = undefined; stream[kOwner] = undefined; stream[kType] = undefined; stream.close(); @@ -139,7 +141,7 @@ function freeStream(stream) { } function onread(nread, buffer) { - const stream = this[kOwner]; + var stream = this[kOwner]; unrefTimer(this); if (nread > 0) { @@ -174,31 +176,36 @@ function onread(nread, buffer) { } function onRstStream(id, code) { - this[kOwner].emit('rststream', id, code); + var owner = this[kOwner]; + owner.emit('rststream', id, code); } function onGoaway(code, lastProcStreamID) { - this[kOwner].emit('goaway', code, lastProcStreamID); + var owner = this[kOwner]; + owner.emit('goaway', code, lastProcStreamID); } function onHeaders(handle, flags, headers, category) { var stream = handle[kOwner]; + var owner = this[kOwner]; if (!stream) { - const id = handle.getId(); - stream = new Http2Stream(this[kOwner], id, {}); + var id = handle.getId(); + stream = new Http2Stream(owner, id, {}); stream._handle = handle; - this[kOwner][kStreams].set(id, stream); + owner[kStreams].set(id, stream); } - this[kOwner].emit('headers', stream, flags, headers, category); + owner.emit('headers', stream, flags, headers, category); } function onStreamClose(id, code) { - const stream = this[kOwner][kStreams].get(id); + var owner = this[kOwner]; + var stream = owner[kStreams].get(id); if (stream) { - this[kOwner].emit('streamClose', stream, code); - this[kOwner][kStreams].delete(id); + owner.emit('streamClose', stream, code); + owner[kStreams].delete(id); freeStream(stream[kHandle]); } } function onError(error) { - this[kOwner].emit('error', error); + var owner = this[kOwner]; + owner.emit('error', error); } function unrefTimer(item) { @@ -282,8 +289,8 @@ class Http2Stream extends Duplex { if (!(handle instanceof http2.Http2Stream)) throw new TypeError('handle must be an Http2Stream'); this[kHandle] = handle; - this[kHandle].onread = onread; - this[kHandle][kOwner] = this; + handle.onread = onread; + handle[kOwner] = this; this.emit('handle', handle); } @@ -305,7 +312,7 @@ class Http2Stream extends Duplex { } get state() { - const obj = {}; + var obj = {}; if (this[kHandle]) this[kHandle].getState(obj); return obj; @@ -414,13 +421,13 @@ class Http2Stream extends Duplex { _write(data, encoding, cb) { if (this[kHandle]) { unrefTimer(this); - const req = new WriteWrap(); + var req = new WriteWrap(); req.handle = this[kHandle]; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; - const enc = data instanceof Buffer ? 'buffer' : encoding; - const err = createWriteReq(req, this[kHandle], data, enc); + var enc = data instanceof Buffer ? 'buffer' : encoding; + var err = createWriteReq(req, this[kHandle], data, enc); if (err) throw util._errnoException(err, 'write', req.error); this._bytesDispatched += req.bytes; @@ -432,18 +439,18 @@ class Http2Stream extends Duplex { _writev(data, cb) { if (this[kHandle]) { unrefTimer(this); - const req = new WriteWrap(); + var req = new WriteWrap(); req.handle = this[kHandle]; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; - const chunks = new Array(data.length << 1); + var chunks = new Array(data.length << 1); for (var i = 0; i < data.length; i++) { - const entry = data[i]; + var entry = data[i]; chunks[i * 2] = entry.chunk; chunks[i * 2 + 1] = entry.encoding; } - const err = this[kHandle].writev(req, chunks); + var err = this[kHandle].writev(req, chunks); if (err) throw util._errnoException(err, 'write', req.error); } else { @@ -462,7 +469,7 @@ class Http2Stream extends Duplex { function afterShutdown() {} function onHandleFinish() { - const req = new ShutdownWrap(); + var req = new ShutdownWrap(); req.oncomplete = afterShutdown; req.handle = this[kHandle]; this[kHandle].shutdown(req); @@ -495,9 +502,10 @@ class Http2Session extends EventEmitter { super(); this[kType] = type; this[kStreams] = new Map(); - this[kHandle] = sessions.alloc(); - this[kHandle][kOwner] = this; - this[kHandle].reinitialize(type, options, socket._handle._externalStream); + var handle = sessions.alloc(); + this[kHandle] = handle; + handle[kOwner] = this; + handle.reinitialize(type, options, socket._handle._externalStream); this[kSocket] = socket; } @@ -520,7 +528,7 @@ class Http2Session extends EventEmitter { } get state() { - const obj = {}; + var obj = {}; if (this[kHandle]) this[kHandle].getState(obj); return obj; @@ -598,7 +606,8 @@ function initHttp2OutgoingOptions(options) { } function incomingOnRead(nread, buffer) { - const stream = this[kOwner][kRequest]; + var owner = this[kOwner]; + var stream = owner[kRequest]; unrefTimer(this); if (nread > 0) { @@ -636,7 +645,8 @@ function incomingOnRead(nread, buffer) { // TODO(jasnell): This is currently incomplete class Http2Incoming extends Readable { constructor(stream, headers, options) { - super(initHttp2IncomingOptions(options)); + options = initHttp2IncomingOptions(options); + super(options); if (!(stream instanceof Http2Stream)) throw new TypeError('stream argument must be an Http2Stream instance'); if (typeof headers !== 'object') @@ -644,7 +654,8 @@ class Http2Incoming extends Readable { this[kStream] = stream; this[kHeaders] = headers; this[kFinished] = false; - this[kStream][kHandle].onread = incomingOnRead; + var handle = stream[kHandle]; + handle.onread = incomingOnRead; } get finished() { @@ -690,29 +701,36 @@ class Http2ServerRequest extends Http2Incoming { } get method() { - return this.headers[constants.HTTP2_HEADER_METHOD]; + return this[kHeaders][constants.HTTP2_HEADER_METHOD]; } get authority() { - return this.headers[constants.HTTP2_HEADER_AUTHORITY]; + return this[kHeaders][constants.HTTP2_HEADER_AUTHORITY]; } get scheme() { - return this.headers[constants.HTTP2_HEADER_SCHEME]; + return this[kHeaders][constants.HTTP2_HEADER_SCHEME]; } get url() { - return this.headers[constants.HTTP2_HEADER_PATH]; + return this[kHeaders][constants.HTTP2_HEADER_PATH]; } } function onHttp2OutgoingPipe() { if (!this[kHeadersSent]) { - const beginSend = this[kBeginSend]; + var beginSend = this[kBeginSend]; beginSend.call(this); } } +function onHttp2OutgoingFinish() { + this[kFinished] = true; + this[kStream].end(); + var beginSend = this[kBeginSend]; + beginSend.call(this); +} + // Represents an outbound HTTP message. class Http2Outgoing extends Writable { constructor(stream, options) { @@ -722,12 +740,7 @@ class Http2Outgoing extends Writable { this[kHeaders] = []; this[kHeadersSent] = false; this.on('pipe', onHttp2OutgoingPipe); - this.on('finish', () => { - this[kFinished] = true; - this[kStream].end(); - const beginSend = this[kBeginSend]; - beginSend.call(this); - }); + this.on('finish', onHttp2OutgoingFinish); } get stream() { @@ -774,7 +787,7 @@ class Http2Outgoing extends Writable { if (value === undefined || value === null) { throw new TypeError('Value must not be undefined or null'); } - const stream = this[kStream]; + var stream = this[kStream]; stream.addTrailer(name, value, Boolean(noindex)); return this; } @@ -791,7 +804,7 @@ class Http2Outgoing extends Writable { addTrailers(headers) { if (!headers) return; - const keys = Object.keys(headers); + var keys = Object.keys(headers); for (var i = 0; i < keys.length; i++) this.setTrailer(keys[i], headers[keys[i]]); return this; @@ -807,8 +820,8 @@ class Http2Outgoing extends Writable { _write(chunk, encoding, callback) { if (typeof chunk === 'string') chunk = Buffer.from(chunk, encoding); - const stream = this[kStream]; - const beginSend = this[kBeginSend]; + var stream = this[kStream]; + var beginSend = this[kBeginSend]; if (this.stream) { beginSend.call(this); stream.write(chunk, encoding, callback); @@ -820,12 +833,13 @@ class Http2Outgoing extends Writable { [kBeginSend]() { if (!this[kHeadersSent]) { - const implicitHeaders = this[kImplicitHeaders]; + var implicitHeaders = this[kImplicitHeaders]; if (typeof implicitHeaders === 'function') { implicitHeaders.call(this); } this[kHeadersSent] = true; - this.stream.respond(this[kHeaders]); + var stream = this[kStream]; + stream.respond(this[kHeaders]); } } } @@ -890,7 +904,7 @@ class Http2ServerResponse extends Http2Outgoing { } createPushResponse() { - const beginSend = this[kBeginSend]; + var beginSend = this[kBeginSend]; if (!this.pushSupported) return; if (!this[kHeadersSent]) { @@ -914,43 +928,43 @@ class Http2PushResponse extends EventEmitter { super(); this[kResponse] = response; this[kHeaders] = Object.create(null); - this.headers[constants.HTTP2_HEADER_METHOD] = 'GET'; - this.headers[constants.HTTP2_HEADER_AUTHORITY] = + this[kHeaders][constants.HTTP2_HEADER_METHOD] = 'GET'; + this[kHeaders][constants.HTTP2_HEADER_AUTHORITY] = response.stream[kRequest].authority; - this.headers[constants.HTTP2_HEADER_SCHEME] = + this[kHeaders][constants.HTTP2_HEADER_SCHEME] = response.stream[kRequest].scheme; } get path() { - return this.headers[constants.HTTP2_HEADER_PATH]; + return this[kHeaders][constants.HTTP2_HEADER_PATH]; } set path(val) { - this.headers[constants.HTTP2_HEADER_PATH] = String(val); + this[kHeaders][constants.HTTP2_HEADER_PATH] = String(val); } get method() { - return this.headers[constants.HTTP2_HEADER_METHOD]; + return this[kHeaders][constants.HTTP2_HEADER_METHOD]; } set method(val) { - this.headers[constants.HTTP2_HEADER_METHOD] = String(val); + this[kHeaders][constants.HTTP2_HEADER_METHOD] = String(val); } get authority() { - return this.headers[constants.HTTP2_HEADER_AUTHORITY]; + return this[kHeaders][constants.HTTP2_HEADER_AUTHORITY]; } set authority(val) { - this.headers[constants.HTTP2_HEADER_AUTHORITY] = String(val); + this[kHeaders][constants.HTTP2_HEADER_AUTHORITY] = String(val); } get scheme() { - return this.headers[constants.HTTP2_HEADER_SCHEME]; + return this[kHeaders][constants.HTTP2_HEADER_SCHEME]; } set scheme(val) { - this.headers[constants.HTTP2_HEADER_SCHEME] = String(val); + this[kHeaders][constants.HTTP2_HEADER_SCHEME] = String(val); } get headers() { @@ -960,19 +974,19 @@ class Http2PushResponse extends EventEmitter { push(callback) { if (typeof callback !== 'function') throw new TypeError('callback must be a function'); - const parent = this[kResponse].stream; - const ret = parent.sendPushPromise(this[kHeaders]); + var parent = this[kResponse].stream; + var ret = parent.sendPushPromise(this[kHeaders]); if (ret) { - const id = ret.getId(); - const stream = new Http2Stream(parent.session, id, {}); + var id = ret.getId(); + var stream = new Http2Stream(parent.session, id, {}); stream._handle = ret; parent.session[kStreams].set(id, stream); stream.readable = false; - const request = + var request = stream[kRequest] = new Http2ServerRequest(stream, this[kHeaders]); - const response = + var response = stream[kResponse] = new Http2ServerResponse(stream, this[kResponse][kOptions]); request[kFinished] = true; @@ -1015,18 +1029,18 @@ function isIllegalConnectionSpecificHeader(name, value) { // the number of expected items up front (it's less // expensive to count than it is to reallocate). function mapToHeaders(map) { - const keys = Object.keys(map); + var keys = Object.keys(map); var size = keys.length; for (var i = 0; i < keys.length; i++) { if (Array.isArray(keys[i])) { size += keys[i].length - 1; } } - const ret = new http2.Http2Headers(size); + var ret = new http2.Http2Headers(size); for (i = 0; i < keys.length; i++) { - const key = keys[i]; - const value = map[key]; + var key = keys[i]; + var value = map[key]; if (Array.isArray(value) && value.length > 0) { for (var k = 0; k < value.length; k++) { ret.add(key, String(value[k])); @@ -1065,7 +1079,7 @@ function socketOnPause() { } function socketOnDrain() { - const needPause = 0 > this._writableState.highWaterMark; + var needPause = 0 > this._writableState.highWaterMark; if (this._paused && !needPause) { this._paused = false; this.resume(); @@ -1073,8 +1087,8 @@ function socketOnDrain() { } function sessionOnStreamClose(stream, code) { - const request = stream[kRequest]; - const response = stream[kResponse]; + var request = stream[kRequest]; + var response = stream[kResponse]; if (request && !request.finished) { request.readable = false; request[kFinished] = true; @@ -1094,9 +1108,9 @@ function sessionOnStreamClose(stream, code) { } function sessionOnError(error) { - const session = this; - const server = session[kServer]; - const socket = session[kSocket]; + var session = this; + var server = session[kServer]; + var socket = session[kSocket]; if (server.listenerCount('sessionError') > 0) { server.emit('sessionError', error); @@ -1106,8 +1120,8 @@ function sessionOnError(error) { } function socketOnTimeout() { - const socket = this; - const server = socket[kServer]; + var socket = this; + var server = socket[kServer]; if (!server.emit('timeout', this)) { // Session timed out, attempt a graceful exit @@ -1138,32 +1152,23 @@ function socketOnceError(error) { } function sessionOnHeaderComplete(stream, flags, headers, category) { - const finished = Boolean(flags & constants.NGHTTP2_FLAG_END_STREAM); - const server = this[kServer]; - // This is a server, so the only header categories supported are - // NGHTTP2_HCAT_REQUEST and NGHGTTP2_HCAT_HEADERS. Other categories - // must result in a Protocol error per the spec. + var finished = Boolean(flags & constants.NGHTTP2_FLAG_END_STREAM); + var server = this[kServer]; + var options = server[kOptions]; var request; var response; switch (category) { case constants.NGHTTP2_HCAT_REQUEST: request = stream[kRequest] = new Http2ServerRequest(stream, headers, - server[kOptions].defaultIncomingOptions); + options.defaultIncomingOptions); response = stream[kResponse] = new Http2ServerResponse(stream, - server[kOptions].defaultOutgoingOptions); - // finished will be true if the header block included flags to end - // the stream (such as when sending a GET request). In such cases, - // mark the kRequest stream finished so no data will be read. + options.defaultOutgoingOptions); if (finished) request[kFinished] = true; if (headers.expect) { - // If there is an expect header that contains 100-continue, - // and the server has a listener for the checkContinue event, - // emit the checkContinue event instead of the request event. - // This behavior matches the current http/1 API. if (/^100-continue$/i.test(headers.expect)) { if (server.listenerCount('checkContinue') > 0) { request[kInFlight] = true; @@ -1172,13 +1177,8 @@ function sessionOnHeaderComplete(stream, flags, headers, category) { break; } response.writeContinue(); - // This falls through to the emit the request event + // fallthrough } else { - // If there is an expect header that contains anything - // other than 100-continue, emit the checkExpectation - // event if there are listeners or automatically return - // a 417 and end the response. This behavior matches the - // current http/1 API if (server.listenerCount('checkExpectation') > 0) { request[kInFlight] = true; server.emit('checkExpectation', request, response); @@ -1190,13 +1190,7 @@ function sessionOnHeaderComplete(stream, flags, headers, category) { break; } } - // Handle CONNECT requests. If there is a connect listener, emit the - // connect event rather than the request event, otherwise RST-STREAM - // with the NGHTTP2_REFUSED_STREAM code. - // TODO(jasnell): Still need to test that this is working correctly. - // To do so we need a client that can send a proper http/2 connect - // request. - if (request.method === 'CONNECT') { + if (headers[constants.HTTP2_HEADER_METHOD] === 'CONNECT') { if (server.listenerCount('connect') > 0) { request[kInFlight] = true; server.emit('connect', request, response); @@ -1212,16 +1206,11 @@ function sessionOnHeaderComplete(stream, flags, headers, category) { break; case constants.NGHTTP2_HCAT_HEADERS: if (!finished) { - // When category === NGHTTP2_HCAT_HEADERS and finished is not - // null, that means an extra HEADERS frame was sent after - // the initial HEADERS frame that opened the request, without the - // end stream flag set. Interstitial headers frames are not permitted - // in the HTTP semantic binding per the HTTP/2 spec stream.protocolError(); return; } - // If finished, that means these are trailing headers - stream[kRequest][kTrailers] = headers; + request = stream[kRequest]; + request[kTrailers] = headers; break; default: stream.protocolError(); @@ -1233,10 +1222,10 @@ function connectionListener(socket) { // highly recommended for http/2 implementations // TODO(jasnell): May want to make this a configuration option? socket.setNoDelay(); - const options = this[kOptions]; + var options = this[kOptions]; // Create the Http2Session instance that is unique to this socket. - const session = createServerSession(options, socket); + var session = createServerSession(options, socket); session[kServer] = this; socket[kServer] = this; this[kSession] = session; @@ -1255,7 +1244,7 @@ function connectionListener(socket) { socket.on('timeout', socketOnTimeout); // Destroy the session if the socket is destroyed - const destroySocket = socket.destroy; + var destroySocket = socket.destroy; socket.destroy = function(error) { session.removeAllListeners(); socket.removeAllListeners(); @@ -1394,12 +1383,12 @@ function clientSessionOnError(client, socket) { } function clientSessionOnHeaderComplete(stream, flags, headers, category) { - const finished = Boolean(flags & constants.NGHTTP2_FLAG_END_STREAM); - const request = stream[kRequest]; + var finished = Boolean(flags & constants.NGHTTP2_FLAG_END_STREAM); + var request = stream[kRequest]; switch (category) { case constants.NGHTTP2_HCAT_RESPONSE: // TODO: Handle various types of responses appropriately - const response = new Http2ClientResponse(stream, headers, {}); + var response = new Http2ClientResponse(stream, headers, {}); stream[kResponse] = response; request.emit('response', response); break; @@ -1420,8 +1409,8 @@ function clientSessionOnHeaderComplete(stream, flags, headers, category) { } function clientSessionOnStreamClose(stream, code) { - const request = stream[kRequest]; - const response = stream[kResponse]; + var request = stream[kRequest]; + var response = stream[kResponse]; if (response && !response.finished) { response.readable = false; response[kFinished] = true; @@ -1466,10 +1455,10 @@ class Http2ClientSession extends EventEmitter { constructor(options, callback) { super(); this[kOptions] = initializeClientOptions(options); - const socket = acquireSocket(options, () => { + var socket = acquireSocket(options, () => { this[kSocket] = socket; - const session = this[kSession] = createClientSession(options, socket); + var session = this[kSession] = createClientSession(options, socket); // TODO remove this socket.once('error', console.log); socket.on('resume', socketOnResume); @@ -1501,8 +1490,8 @@ class Http2ClientSession extends EventEmitter { request(options, callback) { options = options || {}; - const stream = new Http2Stream(this[kSession], -1, options); - const req = new Http2ClientRequest(stream, + var stream = new Http2Stream(this[kSession], -1, options); + var req = new Http2ClientRequest(stream, initializeClientOptions(options), callback); return req; @@ -1551,7 +1540,7 @@ class Http2ClientRequest extends Http2Outgoing { var authority = options.hostname; if (options.port) authority += `:${options.port}`; - const headers = this[kHeaders] = Object.create(null); + var headers = this[kHeaders] = Object.create(null); headers[constants.HTTP2_HEADER_SCHEME] = options.protocol.slice(0, options.protocol.length - 1); @@ -1565,7 +1554,7 @@ class Http2ClientRequest extends Http2Outgoing { setHeader(name, value) { name = String(name).toLowerCase().trim(); - const existing = this[kHeaders][name]; + var existing = this[kHeaders][name]; if (existing) { if (Array.isArray(existing)) { existing.push(String(value)); @@ -1581,7 +1570,7 @@ class Http2ClientRequest extends Http2Outgoing { if (!this[kTrailers]) this[kTrailers] = Object.create(null); name = String(name).toLowerCase().trim(); - const existing = this[kTrailers][name]; + var existing = this[kTrailers][name]; if (existing) { if (Array.isArray(existing)) { existing.push(String(value)); @@ -1596,9 +1585,9 @@ class Http2ClientRequest extends Http2Outgoing { [kBeginSend]() { if (!this[kHeadersSent]) { this[kHeadersSent] = true; - const stream = this[kStream]; + var stream = this[kStream]; this[kImplicitHeaders](); - const _handle = stream.session.request(mapToHeaders(this[kHeaders]), true); + var _handle = stream.session.request(mapToHeaders(this[kHeaders]), true); if (_handle instanceof http2.Http2Stream) { this[kId] = _handle.getId(); stream.once('handle', addTrailers); @@ -1613,11 +1602,11 @@ class Http2ClientRequest extends Http2Outgoing { } function addTrailers() { - const request = this[kRequest]; + var request = this[kRequest]; if (request[kTrailers]) { // key is coerced on a string on set for (var key in request[kTrailers]) { - const value = request[kTrailers][key]; + var value = request[kTrailers][key]; if (Array.isArray(value) && value.length > 0) { for (var i = 0; i < value.length; i++) this.addTrailer(key, String(value[i])); @@ -1634,7 +1623,7 @@ class Http2ClientResponse extends Http2Incoming { } get status() { - return this.headers[constants.HTTP2_HEADER_STATUS] | 0; + return this[kHeaders][constants.HTTP2_HEADER_STATUS] | 0; } } From 169b72cb18f0ca6e49012b655bf43f3879190181 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 13:25:12 -0800 Subject: [PATCH 09/13] Avoid deoptimization --- lib/internal/http2.js | 158 ++++++++++++++++++++++++++---------------- 1 file changed, 97 insertions(+), 61 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index f8d5040459..5571d5d0c3 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -266,7 +266,8 @@ function maybeDestroyStream(stream) { if ((!stream.writable && !stream._writableState.length)) { timers.unenroll(stream); - stream.session[kStreams].delete(stream.id); + var session = stream[kSession]; + session[kStreams].delete(stream[kId]); freeStream(stream[kHandle]); } } @@ -659,7 +660,7 @@ class Http2Incoming extends Readable { } get finished() { - return this.stream === undefined || this[kFinished]; + return this[kStream] === undefined || this[kFinished]; } get stream() { @@ -684,13 +685,16 @@ class Http2Incoming extends Readable { // Set the timeout on the underlying Http2Stream object setTimeout(msecs, callback) { - if (!this.stream) return; - this.stream.setTimeout(msecs, callback); + var stream = this[kStream]; + if (!stream) return; + stream.setTimeout(msecs, callback); return this; } _read(n) { - this[kStream][kHandle].readStart(); + var stream = this[kStream]; + var handle = stream[kHandle]; + handle.readStart(); } } @@ -701,19 +705,23 @@ class Http2ServerRequest extends Http2Incoming { } get method() { - return this[kHeaders][constants.HTTP2_HEADER_METHOD]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_METHOD]; } get authority() { - return this[kHeaders][constants.HTTP2_HEADER_AUTHORITY]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_AUTHORITY]; } get scheme() { - return this[kHeaders][constants.HTTP2_HEADER_SCHEME]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_SCHEME]; } get url() { - return this[kHeaders][constants.HTTP2_HEADER_PATH]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_PATH]; } } @@ -748,7 +756,8 @@ class Http2Outgoing extends Writable { } get finished() { - return this.stream === undefined || this[kFinished]; + var stream = this[kStream]; + return stream === undefined || this[kFinished]; } get headersSent() { @@ -756,10 +765,11 @@ class Http2Outgoing extends Writable { } setHeader(name, value, noindex) { - if (this.headersSent) + var stream = this[kStream]; + if (this[kHeadersSent]) throw new Error( 'Cannot set headers after the HTTP message has been sent'); - if (!this.stream) + if (!stream) throw new Error('Cannot set header on a closed stream'); name = String(name).toLowerCase().trim(); if (isPseudoHeader(name)) @@ -774,10 +784,11 @@ class Http2Outgoing extends Writable { } setTrailer(name, value, noindex) { - if (this.headersSent) + var stream = this[kStream]; + if (this[kHeadersSent]) throw new Error( 'Cannot set trailers after the HTTP message has been sent'); - if (!this.stream) + if (!stream) throw new Error('Cannot set trailer on a closed stream'); name = String(name).toLowerCase().trim(); if (isPseudoHeader(name)) @@ -787,7 +798,6 @@ class Http2Outgoing extends Writable { if (value === undefined || value === null) { throw new TypeError('Value must not be undefined or null'); } - var stream = this[kStream]; stream.addTrailer(name, value, Boolean(noindex)); return this; } @@ -812,8 +822,9 @@ class Http2Outgoing extends Writable { // Set the timeout on the underlying Http2Stream object setTimeout(msecs, callback) { - if (!this.stream) return; - this.stream.setTimeout(msecs, callback); + var stream = this[kStream]; + if (!stream) return; + stream.setTimeout(msecs, callback); return this; } @@ -822,7 +833,7 @@ class Http2Outgoing extends Writable { chunk = Buffer.from(chunk, encoding); var stream = this[kStream]; var beginSend = this[kBeginSend]; - if (this.stream) { + if (stream) { beginSend.call(this); stream.write(chunk, encoding, callback); } else { @@ -877,13 +888,16 @@ class Http2ServerResponse extends Http2Outgoing { } get pushSupported() { - if (!this.stream) return false; - return this.stream.session.remoteSettings.enablePush; + var stream = this[kStream]; + if (!stream) return false; + var session = stream[kSession]; + return session.remoteSettings.enablePush; } writeContinue() { - if (this.stream) { - this.stream.sendContinue(); + var stream = this[kStream]; + if (stream) { + stream.sendContinue(); } } @@ -915,9 +929,10 @@ class Http2ServerResponse extends Http2Outgoing { [kImplicitHeaders]() { // Set implicit headers.. will be called by [kBeginSend]() + var headers = this[kHeaders]; if (this.sendDate) - this[kHeaders].unshift(['date', utcDate()]); - this[kHeaders].unshift([constants.HTTP2_HEADER_STATUS, this.statusCode]); + headers.unshift(['date', utcDate()]); + headers.unshift([constants.HTTP2_HEADER_STATUS, this[kStatusCode]]); } } @@ -927,44 +942,54 @@ class Http2PushResponse extends EventEmitter { constructor(response) { super(); this[kResponse] = response; - this[kHeaders] = Object.create(null); - this[kHeaders][constants.HTTP2_HEADER_METHOD] = 'GET'; - this[kHeaders][constants.HTTP2_HEADER_AUTHORITY] = - response.stream[kRequest].authority; - this[kHeaders][constants.HTTP2_HEADER_SCHEME] = - response.stream[kRequest].scheme; + var headers = Object.create(null); + var stream = response[kStream]; + this[kHeaders] = headers; + headers[constants.HTTP2_HEADER_METHOD] = 'GET'; + headers[constants.HTTP2_HEADER_AUTHORITY] = + stream[kRequest].authority; + headers[constants.HTTP2_HEADER_SCHEME] = + stream[kRequest].scheme; } get path() { - return this[kHeaders][constants.HTTP2_HEADER_PATH]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_PATH]; } set path(val) { - this[kHeaders][constants.HTTP2_HEADER_PATH] = String(val); + var headers = this[kHeaders]; + headers[constants.HTTP2_HEADER_PATH] = String(val); } get method() { - return this[kHeaders][constants.HTTP2_HEADER_METHOD]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_METHOD]; } set method(val) { - this[kHeaders][constants.HTTP2_HEADER_METHOD] = String(val); + var headers = this[kHeaders]; + headers[constants.HTTP2_HEADER_METHOD] = String(val); } get authority() { - return this[kHeaders][constants.HTTP2_HEADER_AUTHORITY]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_AUTHORITY]; } set authority(val) { - this[kHeaders][constants.HTTP2_HEADER_AUTHORITY] = String(val); + var headers = this[kHeaders]; + headers[constants.HTTP2_HEADER_AUTHORITY] = String(val); } get scheme() { - return this[kHeaders][constants.HTTP2_HEADER_SCHEME]; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_SCHEME]; } set scheme(val) { - this[kHeaders][constants.HTTP2_HEADER_SCHEME] = String(val); + var headers = this[kHeaders]; + headers[constants.HTTP2_HEADER_SCHEME] = String(val); } get headers() { @@ -974,21 +999,24 @@ class Http2PushResponse extends EventEmitter { push(callback) { if (typeof callback !== 'function') throw new TypeError('callback must be a function'); - var parent = this[kResponse].stream; - var ret = parent.sendPushPromise(this[kHeaders]); + var res = this[kResponse]; + var parent = res[kStream]; + var headers = this[kHeaders]; + var ret = parent.sendPushPromise(headers); if (ret) { var id = ret.getId(); - var stream = new Http2Stream(parent.session, id, {}); + var stream = new Http2Stream(parent[kSession], id, {}); + var session = parent[kSession]; stream._handle = ret; - parent.session[kStreams].set(id, stream); + session[kStreams].set(id, stream); stream.readable = false; var request = stream[kRequest] = - new Http2ServerRequest(stream, this[kHeaders]); + new Http2ServerRequest(stream, headers); var response = stream[kResponse] = - new Http2ServerResponse(stream, this[kResponse][kOptions]); + new Http2ServerResponse(stream, res[kOptions]); request[kFinished] = true; request[kInFlight] = true; callback(request, response); @@ -1385,10 +1413,10 @@ function clientSessionOnError(client, socket) { function clientSessionOnHeaderComplete(stream, flags, headers, category) { var finished = Boolean(flags & constants.NGHTTP2_FLAG_END_STREAM); var request = stream[kRequest]; + var response; switch (category) { case constants.NGHTTP2_HCAT_RESPONSE: - // TODO: Handle various types of responses appropriately - var response = new Http2ClientResponse(stream, headers, {}); + response = new Http2ClientResponse(stream, headers, {}); stream[kResponse] = response; request.emit('response', response); break; @@ -1397,7 +1425,8 @@ function clientSessionOnHeaderComplete(stream, flags, headers, category) { stream.protocolError(); return; } - stream[kResponse][kTrailers] = headers; + response = stream[kResponse]; + response[kTrailers] = headers; break; case constants.NGHTTP2_HCAT_PUSH_PROMISE: // TODO(jasnell): Complete this @@ -1554,31 +1583,33 @@ class Http2ClientRequest extends Http2Outgoing { setHeader(name, value) { name = String(name).toLowerCase().trim(); - var existing = this[kHeaders][name]; + var headers = this[kHeaders]; + var existing = headers[name]; if (existing) { if (Array.isArray(existing)) { existing.push(String(value)); } else { - this[kHeaders][name] = [existing, value]; + headers[name] = [existing, value]; } } else { - this[kHeaders][name] = value; + headers[name] = value; } } setTrailer(name, value) { - if (!this[kTrailers]) - this[kTrailers] = Object.create(null); + var trailers = this[kTrailers]; + if (!trailers) + trailers = this[kTrailers] = Object.create(null); name = String(name).toLowerCase().trim(); - var existing = this[kTrailers][name]; + var existing = trailers[name]; if (existing) { if (Array.isArray(existing)) { existing.push(String(value)); } else { - this[kTrailers][name] = [existing, value]; + trailers[name] = [existing, value]; } } else { - this[kTrailers][name] = value; + trailers[name] = value; } } @@ -1586,8 +1617,11 @@ class Http2ClientRequest extends Http2Outgoing { if (!this[kHeadersSent]) { this[kHeadersSent] = true; var stream = this[kStream]; - this[kImplicitHeaders](); - var _handle = stream.session.request(mapToHeaders(this[kHeaders]), true); + var session = stream[kSession]; + var implicitHeaders = this[kImplicitHeaders]; + if (typeof implicitHeaders === 'function') + implicitHeaders.call(this); + var _handle = session.request(mapToHeaders(this[kHeaders]), true); if (_handle instanceof http2.Http2Stream) { this[kId] = _handle.getId(); stream.once('handle', addTrailers); @@ -1603,10 +1637,11 @@ class Http2ClientRequest extends Http2Outgoing { function addTrailers() { var request = this[kRequest]; - if (request[kTrailers]) { + var trailers = request[kTrailers]; + if (trailers) { // key is coerced on a string on set - for (var key in request[kTrailers]) { - var value = request[kTrailers][key]; + for (var key in trailers) { + var value = trailers[key]; if (Array.isArray(value) && value.length > 0) { for (var i = 0; i < value.length; i++) this.addTrailer(key, String(value[i])); @@ -1623,7 +1658,8 @@ class Http2ClientResponse extends Http2Incoming { } get status() { - return this[kHeaders][constants.HTTP2_HEADER_STATUS] | 0; + var headers = this[kHeaders]; + return headers[constants.HTTP2_HEADER_STATUS] | 0; } } From aa5caec3301f59f861768b0ecdff6779fbad0761 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 9 Dec 2016 13:46:44 -0800 Subject: [PATCH 10/13] Avoid more deoptimizations --- lib/internal/http2.js | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index 5571d5d0c3..9ae8a56a64 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -118,14 +118,12 @@ function initSessionHandle() { } function freeSession(session) { - if (session) { - session.reset(); - var owner = session[kOwner]; - owner[kHandle] = undefined; - session[kOwner] = undefined; - if (sessions.free(session) === false) - session.close(); - } + session.reset(); + var owner = session[kOwner]; + owner[kHandle] = undefined; + session[kOwner] = undefined; + if (sessions.free(session) === false) + session.close(); } function freeStream(stream) { @@ -766,6 +764,7 @@ class Http2Outgoing extends Writable { setHeader(name, value, noindex) { var stream = this[kStream]; + var headers = this[kHeaders]; if (this[kHeadersSent]) throw new Error( 'Cannot set headers after the HTTP message has been sent'); @@ -779,7 +778,7 @@ class Http2Outgoing extends Writable { if (value === undefined || value === null) { throw new TypeError('Value must not be undefined or null'); } - this[kHeaders].push([name, value, Boolean(noindex)]); + headers.push([name, value, Boolean(noindex)]); return this; } @@ -1245,6 +1244,18 @@ function sessionOnHeaderComplete(stream, flags, headers, category) { } } +function socketDestroy(destroySocket, session) { + return function(error) { + session.removeAllListeners(); + this.removeAllListeners(); + freeSession(session[kHandle]); + this.destroy = destroySocket; + destroySocket.call(this, error); + this[kServer] = undefined; + session[kServer] = undefined; + }; +} + function connectionListener(socket) { // Turn off the Nagle's for this socket... // highly recommended for http/2 implementations @@ -1273,15 +1284,7 @@ function connectionListener(socket) { // Destroy the session if the socket is destroyed var destroySocket = socket.destroy; - socket.destroy = function(error) { - session.removeAllListeners(); - socket.removeAllListeners(); - freeSession(session[kHandle]); - socket.destroy = destroySocket; - destroySocket.call(socket, error); - socket[kServer] = undefined; - session[kServer] = undefined; - }; + socket.destroy = socketDestroy(destroySocket, session); socket.once('error', socketOnceError); socket.on('resume', socketOnResume); socket.on('pause', socketOnPause); From 2a6dd7bb8e331090b28f3509dfd82edfcf18a614 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 10 Dec 2016 10:39:25 -0800 Subject: [PATCH 11/13] Refactor settings to be more efficient --- lib/internal/http2.js | 74 ++++--- src/env.h | 1 - src/node_http2.cc | 442 +++++++++++++++--------------------------- src/node_http2.h | 108 +---------- 4 files changed, 207 insertions(+), 418 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index 9ae8a56a64..e54609a7f5 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -543,21 +543,28 @@ class Http2Session extends EventEmitter { this[kHandle].setLocalWindowSize(size); } - get remoteSettings() { - if (this[kHandle]) - return this[kHandle].getRemoteSettings(); - } - - get localSettings() { - if (this[kHandle]) - return this[kHandle].getLocalSettings(); - } - - set localSettings(settings) { - if (!(settings instanceof http2.Http2Settings)) - throw new TypeError('settings must be an instance of Http2Settings'); - if (this[kHandle]) { - this[kHandle].setLocalSettings(settings); + getRemoteSettings() { + var holder = {}; + var handle = this[kHandle]; + if (handle) + handle.getRemoteSettings(holder); + return holder; + } + + getLocalSettings() { + var holder = {}; + var handle = this[kHandle]; + if (handle) + handle.getLocalSettings(holder); + return holder; + } + + setLocalSettings(settings) { + if (typeof settings !== 'object') + throw new TypeError('settings must be an object'); + var handle = this[kHandle]; + if (handle) { + handle.setLocalSettings(settings); } } @@ -1291,17 +1298,15 @@ function connectionListener(socket) { socket.on('drain', socketOnDrain); session.on('headers', sessionOnHeaderComplete); session.on('streamClose', sessionOnStreamClose); - session.localSettings = options.settings; + session.setLocalSettings(options.settings); } function initializeOptions(options) { options = options || {}; options.allowHalfOpen = true; - options.settings = options.settings || new http2.Http2Settings(); - if (!(options.settings instanceof http2.Http2Settings)) { - throw new TypeError( - 'options.settings must be an instance of Http2Settings'); - } + options.settings = options.settings || {}; + if (typeof options.settings !== 'object') + throw new TypeError('options.settings must be an object'); if (!options.defaultIncomingOptions || typeof options.defaultIncomingOptions !== 'object') { options.defaultIncomingOptions = initHttp2IncomingOptions({}); @@ -1670,14 +1675,29 @@ function createClient(options, callback) { return new Http2ClientSession(options, callback); } +function getDefaultSettings() { + var holder = {}; + http2.getDefaultSettings(holder); + return holder; +} + +function getPackedSettings(obj) { + obj = obj || {}; + if (typeof obj !== 'object') + throw new TypeError('settings must be an object'); + return http2.packSettings(obj); +} + // Exports module.exports = { + constants, + getDefaultSettings, + getPackedSettings, + createClient, + createServer, + createSecureServer, + createServerSession, + createClientSession, get: Http2ClientSession.get, request: Http2ClientSession.request, - Http2Settings: http2.Http2Settings, - createClient: createClient, - createServer: createServer, - createSecureServer: createSecureServer, - createServerSession: createServerSession, - createClientSession: createClientSession }; diff --git a/src/env.h b/src/env.h index 860f1ea00b..a13836cb6b 100644 --- a/src/env.h +++ b/src/env.h @@ -249,7 +249,6 @@ namespace node { V(fs_stats_constructor_function, v8::Function) \ V(generic_internal_field_template, v8::ObjectTemplate) \ V(http2headers_constructor_template, v8::FunctionTemplate) \ - V(http2settings_constructor_template, v8::FunctionTemplate) \ V(http2stream_object, v8::Object) \ V(jsstream_constructor_template, v8::FunctionTemplate) \ V(module_load_list_array, v8::Array) \ diff --git a/src/node_http2.cc b/src/node_http2.cc index ce4b38dab2..905db18c44 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -19,6 +19,7 @@ namespace node { using v8::Array; +using v8::Boolean; using v8::Context; using v8::External; using v8::Function; @@ -51,232 +52,24 @@ Http2Options::Http2Options(Environment* env, Local options) { nghttp2_option_new(&options_); if (options->IsObject()) { Local opts = options.As(); + Local context = env->context(); + Isolate* isolate = env->isolate(); #define V(obj, name, fn, type) \ - { \ - Local val = obj->Get(FIXED_ONE_BYTE_STRING(env->isolate(), name)); \ - if (!val.IsEmpty() && !val->IsUndefined()) \ - fn(val->type##Value()); \ - } + do { \ + Local str = FIXED_ONE_BYTE_STRING(isolate, name); \ + if (obj->Has(context, str).FromJust()) { \ + Local val = obj->Get(context, str).ToLocalChecked(); \ + if (!val->IsUndefined() && !val->IsNull()) \ + fn(val->type##Value()); \ + } \ + } while (0); OPTIONS(opts, V) #undef V } } #undef OPTIONS -// Http2Settings statics - -// Utility typedef used to abstract getting remote or local -// settings from the nghttp2_session instance. -typedef uint32_t(*get_setting)(nghttp2_session* session, - nghttp2_settings_id id); -Http2Settings::Http2Settings(Environment* env, - Local wrap, - Http2Session* session, - bool localSettings) : - BaseObject(env, wrap) { - MakeWeak(this); - - if (session != nullptr) { - // When initialized using an existing Http2Session instance, - // fetch the currently established settings and fill in the - // internal map. - get_setting fn = - localSettings ? - nghttp2_session_get_local_settings : - nghttp2_session_get_remote_settings; - Set(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, - fn(**session, NGHTTP2_SETTINGS_HEADER_TABLE_SIZE)); - Set(NGHTTP2_SETTINGS_ENABLE_PUSH, - fn(**session, NGHTTP2_SETTINGS_ENABLE_PUSH)); - Set(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - fn(**session, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS)); - Set(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - fn(**session, NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE)); - Set(NGHTTP2_SETTINGS_MAX_FRAME_SIZE, - fn(**session, NGHTTP2_SETTINGS_MAX_FRAME_SIZE)); - Set(NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, - fn(**session, NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE)); - } -} - -void Http2Settings::New(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - if (!args.IsConstructCall()) - return env->ThrowTypeError("Class constructor Http2Settings cannot " - "be invoked without 'new'"); - new Http2Settings(env, args.This()); -} - -// Used to fill in the spec defined initial values for each setting. -void Http2Settings::Defaults(const FunctionCallbackInfo& args) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, args.Holder()); - settings->settings_.clear(); - settings->Set(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, - DEFAULT_SETTINGS_HEADER_TABLE_SIZE); - settings->Set(NGHTTP2_SETTINGS_ENABLE_PUSH, - DEFAULT_SETTINGS_ENABLE_PUSH); - settings->Set(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE); - settings->Set(NGHTTP2_SETTINGS_MAX_FRAME_SIZE, - DEFAULT_SETTINGS_MAX_FRAME_SIZE); -} - -// Reset the settings object by clearing the internal map -void Http2Settings::Reset(const FunctionCallbackInfo& args) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, args.Holder()); - settings->settings_.clear(); -} - -// Serializes the settings object into a Buffer instance that -// would be suitable, for instance, for creating the Base64 -// output for an HTTP2-Settings header field. -void Http2Settings::Pack(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - HandleScope scope(env->isolate()); - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, args.Holder()); - std::vector entries; - settings->CollectSettings(&entries); - size_t len = entries.size() * 6; - MaybeStackBuffer buf(len); - ssize_t ret = - nghttp2_pack_settings_payload( - reinterpret_cast(*buf), len, &entries[0], entries.size()); - if (ret >= 0) { - args.GetReturnValue().Set( - Buffer::Copy(env, *buf, len).ToLocalChecked()); - } -} - -void Http2Settings::GetHeaderTableSize( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->Find(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, info); -} - -void Http2Settings::SetHeaderTableSize( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) - settings->Erase(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE); - else - settings->Set(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, value->Uint32Value()); -} - -void Http2Settings::GetEnablePush( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->FindBoolean(NGHTTP2_SETTINGS_ENABLE_PUSH, info); -} - -void Http2Settings::SetEnablePush( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) - settings->Erase(NGHTTP2_SETTINGS_ENABLE_PUSH); - else - settings->Set(NGHTTP2_SETTINGS_ENABLE_PUSH, value->BooleanValue() ? 1 : 0); -} - -void Http2Settings::GetMaxConcurrentStreams( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->Find(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, info); -} - -void Http2Settings::SetMaxConcurrentStreams( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) { - settings->Erase(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); - } else { - settings->Set(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - value->Uint32Value()); - } -} - -void Http2Settings::GetInitialWindowSize( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->Find(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, info); -} - -void Http2Settings::SetInitialWindowSize( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) - settings->Erase(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE); - else - settings->Set(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - MIN(MAX_INITIAL_WINDOW_SIZE, value->Uint32Value())); -} - -void Http2Settings::GetMaxFrameSize( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->Find(NGHTTP2_SETTINGS_MAX_FRAME_SIZE, info); -} - -void Http2Settings::SetMaxFrameSize( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) { - settings->Erase(NGHTTP2_SETTINGS_MAX_FRAME_SIZE); - } else { - settings->Set( - NGHTTP2_SETTINGS_MAX_FRAME_SIZE, - MAX(MIN(value->Uint32Value(), MAX_MAX_FRAME_SIZE), MIN_MAX_FRAME_SIZE)); - } -} - -void Http2Settings::GetMaxHeaderListSize( - Local property, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - settings->Find(NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, info); -} -void Http2Settings::SetMaxHeaderListSize( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Http2Settings* settings; - ASSIGN_OR_RETURN_UNWRAP(&settings, info.Holder()); - if (value->IsUndefined()) - settings->Erase(NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE); - else - settings->Set(NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, value->Uint32Value()); -} - - // Http2Headers statics // Create a new Http2Headers object. The first argument is the @@ -680,15 +473,18 @@ int Http2Stream::DoWrite(WriteWrap* w, w->Dispatched(); w->Done(0); + return 0; } bool Http2Stream::IsAlive() { return true; - //return nghttp2_stream_get_state(**this) != NGHTTP2_STREAM_STATE_CLOSED; + // TODO(jasnell): Revisit this + // return nghttp2_stream_get_state(**this) != NGHTTP2_STREAM_STATE_CLOSED; } bool Http2Stream::IsClosing() { + // TODO(jasnell): Revisit this return false; } @@ -1232,18 +1028,55 @@ void Http2Session::SetLocalWindowSize(const FunctionCallbackInfo& args) { **session, NGHTTP2_FLAG_NONE, 0, args[0]->Int32Value()); } +#define SETTINGS(V) \ + V("headerTableSize", NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, \ + Integer, NewFromUnsigned) \ + V("maxConcurrentStreams", NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, \ + Integer, NewFromUnsigned) \ + V("initialWindowSize", NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, \ + Integer, NewFromUnsigned) \ + V("maxFrameSize", NGHTTP2_SETTINGS_MAX_FRAME_SIZE, \ + Integer, NewFromUnsigned) \ + V("maxHeaderListSize", NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE, \ + Integer, NewFromUnsigned) \ + V("enablePush", NGHTTP2_SETTINGS_ENABLE_PUSH, \ + Boolean, New) + +typedef uint32_t(*get_setting)(nghttp2_session* session, + nghttp2_settings_id id); +void GetSettings(Environment* env, + nghttp2_session* session, + get_setting fn, + Local obj) { + Local context = env->context(); + Isolate* isolate = env->isolate(); +#define V(name, id, type, c) \ + obj->Set(context, \ + FIXED_ONE_BYTE_STRING(isolate, name), \ + type::c(isolate, fn(session, id))).FromJust(); + SETTINGS(V) +#undef V +} + void Http2Session::GetLocalSettings( const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Environment* env = session->env(); - HandleScope scope(env->isolate()); - CHECK_EQ(env->http2settings_constructor_template().IsEmpty(), false); - Local constructor = - env->http2settings_constructor_template()->GetFunction(); - CHECK_EQ(constructor.IsEmpty(), false); - Local obj = constructor->NewInstance(env->context()).ToLocalChecked(); - new Http2Settings(env, obj, session, true); + CHECK(args[0]->IsObject()); + Local obj = args[0].As(); + GetSettings(env, **session, nghttp2_session_get_local_settings, obj); + args.GetReturnValue().Set(obj); +} + +void Http2Session::GetRemoteSettings( + const FunctionCallbackInfo& args) { + Http2Session* session; + ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); + Environment* env = session->env(); + CHECK(args[0]->IsObject()); + Local obj = args[0].As(); + GetSettings(env, **session, nghttp2_session_get_remote_settings, obj); args.GetReturnValue().Set(obj); } @@ -1251,34 +1084,31 @@ void Http2Session::SetLocalSettings(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Environment* env = session->env(); + Local context = env->context(); + Isolate* isolate = env->isolate(); + + CHECK(args[0]->IsObject()); + Local obj = args[0].As(); - Http2Settings* settings; - THROW_AND_RETURN_UNLESS_HTTP2SETTINGS(env, args[0]); - ASSIGN_OR_RETURN_UNWRAP(&settings, args[0].As()); std::vector entries; - settings->CollectSettings(&entries); + +#define V(name, id, type, c) \ + do { \ + Local str = FIXED_ONE_BYTE_STRING(isolate, name); \ + if (obj->Has(context, str).FromJust()) { \ + Local val = obj->Get(context, str).ToLocalChecked(); \ + if (!val->IsUndefined() && !val->IsNull()) \ + entries.push_back({id, val->Uint32Value()}); \ + } \ + } while (0); + SETTINGS(V) +#undef V nghttp2_submit_settings(**session, NGHTTP2_FLAG_NONE, &entries[0], entries.size()); session->SendIfNecessary(); } -void Http2Session::GetRemoteSettings( - const FunctionCallbackInfo& args) { - Http2Session* session; - ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - Environment* env = session->env(); - - HandleScope scope(env->isolate()); - CHECK_EQ(env->http2settings_constructor_template().IsEmpty(), false); - Local constructor = - env->http2settings_constructor_template()->GetFunction(); - CHECK_EQ(constructor.IsEmpty(), false); - Local obj = constructor->NewInstance(env->context()).ToLocalChecked(); - new Http2Settings(env, obj, session, false); - args.GetReturnValue().Set(obj); -} - // Signals termination of the nghttp2_session by sending a GOAWAY // frame. The only argument is the goaway error code. void Http2Session::Terminate(const FunctionCallbackInfo& args) { @@ -1357,6 +1187,73 @@ void HttpErrorString(const FunctionCallbackInfo& args) { nghttp2_strerror(args[0]->Uint32Value()))); } +// Serializes the settings object into a Buffer instance that +// would be suitable, for instance, for creating the Base64 +// output for an HTTP2-Settings header field. +void PackSettings(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + Local context = env->context(); + HandleScope scope(env->isolate()); + + CHECK(args[0]->IsObject()); + Local obj = args[0].As(); + std::vector entries; + +#define V(name, id, type, c) \ + do { \ + Local str = FIXED_ONE_BYTE_STRING(isolate, name); \ + if (obj->Has(context, str).FromJust()) { \ + Local val = obj->Get(context, str).ToLocalChecked(); \ + if (!val->IsUndefined() && !val->IsNull()) \ + entries.push_back({id, val->Uint32Value()}); \ + } \ + } while (0); + SETTINGS(V) +#undef V + + const size_t len = entries.size() * 6; + MaybeStackBuffer buf(len); + ssize_t ret = + nghttp2_pack_settings_payload( + reinterpret_cast(*buf), len, &entries[0], entries.size()); + if (ret >= 0) { + args.GetReturnValue().Set( + Buffer::Copy(env, *buf, len).ToLocalChecked()); + } +} + +// Used to fill in the spec defined initial values for each setting. +void GetDefaultSettings(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + Local context = env->context(); + CHECK(args[0]->IsObject()); + Local obj = args[0].As(); + obj->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "headerTableSize"), + Integer::NewFromUnsigned( + isolate, + DEFAULT_SETTINGS_HEADER_TABLE_SIZE)) + .FromJust(); + obj->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "enablePush"), + Boolean::New(isolate, DEFAULT_SETTINGS_ENABLE_PUSH)) + .FromJust(); + obj->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "initialWindowSize"), + Integer::NewFromUnsigned( + isolate, + DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE)) + .FromJust(); + obj->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "maxFrameSize"), + Integer::NewFromUnsigned( + isolate, + DEFAULT_SETTINGS_MAX_FRAME_SIZE)) + .FromJust(); + args.GetReturnValue().Set(obj); +} void Initialize(Local target, Local unused, @@ -1378,9 +1275,6 @@ void Initialize(Local target, Local http2StreamClassName = String::NewFromUtf8(isolate, "Http2Stream", v8::NewStringType::kInternalized).ToLocalChecked(); - Local http2SettingsClassName = - String::NewFromUtf8(isolate, "Http2Settings", - v8::NewStringType::kInternalized).ToLocalChecked(); // Persistent FunctionTemplate for Http2Stream. Instances of this // class are only intended to be created by Http2Session::CreateStream @@ -1411,44 +1305,6 @@ void Initialize(Local target, stream->GetFunction()->NewInstance(env->context()).ToLocalChecked()); target->Set(http2StreamClassName, stream->GetFunction()); - // Http2Settings Template - Local settings = - env->NewFunctionTemplate(Http2Settings::New); - settings->SetClassName(http2SettingsClassName); - settings->InstanceTemplate()->SetInternalFieldCount(1); - - env->SetAccessor(settings, - "headerTableSize", - Http2Settings::GetHeaderTableSize, - Http2Settings::SetHeaderTableSize); - env->SetAccessor(settings, - "enablePush", - Http2Settings::GetEnablePush, - Http2Settings::SetEnablePush); - env->SetAccessor(settings, - "maxConcurrentStreams", - Http2Settings::GetMaxConcurrentStreams, - Http2Settings::SetMaxConcurrentStreams); - env->SetAccessor(settings, - "initialWindowSize", - Http2Settings::GetInitialWindowSize, - Http2Settings::SetInitialWindowSize); - env->SetAccessor(settings, - "maxFrameSize", - Http2Settings::GetMaxFrameSize, - Http2Settings::SetMaxFrameSize); - env->SetAccessor(settings, - "maxHeaderListSize", - Http2Settings::GetMaxHeaderListSize, - Http2Settings::SetMaxHeaderListSize); - env->SetProtoMethod(settings, "setDefaults", Http2Settings::Defaults); - env->SetProtoMethod(settings, "reset", Http2Settings::Reset); - env->SetProtoMethod(settings, "pack", Http2Settings::Pack); - env->set_http2settings_constructor_template(settings); - target->Set(context, - http2SettingsClassName, - settings->GetFunction()).FromJust(); - // Http2Headers Template Local headers = env->NewFunctionTemplate(Http2Headers::New); @@ -1521,6 +1377,14 @@ void Initialize(Local target, NODE_DEFINE_CONSTANT(constants, NGHTTP2_NV_FLAG_NO_INDEX); NODE_DEFINE_CONSTANT(constants, NGHTTP2_ERR_DEFERRED); + NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_HEADER_TABLE_SIZE); + NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_ENABLE_PUSH); + NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE); + NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_MAX_FRAME_SIZE); + NODE_DEFINE_CONSTANT(constants, MAX_MAX_FRAME_SIZE); + NODE_DEFINE_CONSTANT(constants, MIN_MAX_FRAME_SIZE); + NODE_DEFINE_CONSTANT(constants, MAX_INITIAL_WINDOW_SIZE); + #define STRING_CONSTANT(N) NODE_DEFINE_STRING_CONSTANT(constants, #N, N) STRING_CONSTANT(HTTP2_HEADER_STATUS); STRING_CONSTANT(HTTP2_HEADER_METHOD); @@ -1537,6 +1401,10 @@ HTTP_STATUS_CODES(V) DATA_FLAGS(V) #undef V + env->SetMethod(target, "getDefaultSettings", GetDefaultSettings); + env->SetMethod(target, "packSettings", PackSettings); + + target->Set(context, FIXED_ONE_BYTE_STRING(isolate, "constants"), constants).FromJust(); diff --git a/src/node_http2.h b/src/node_http2.h index d43d67dcc4..48c6f22e9f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -214,7 +214,6 @@ class Http2Header; class Http2Session; class Http2Stream; class Http2Priority; -class Http2Settings; void DoEmit(AsyncWrap* emitter, Local name, @@ -300,103 +299,6 @@ class Http2Options { nghttp2_option* options_; }; -class Http2Settings : public BaseObject { - public: - Http2Settings(Environment* env, - Local wrap, - Http2Session* session = nullptr, - bool localSettings = true); - - ~Http2Settings() {} - - static void New(const FunctionCallbackInfo& args); - static void Defaults(const FunctionCallbackInfo& args); - static void Reset(const FunctionCallbackInfo& args); - static void Pack(const FunctionCallbackInfo& args); - - static void GetHeaderTableSize( - Local property, - const PropertyCallbackInfo& info); - static void SetHeaderTableSize( - Local property, - Local value, - const PropertyCallbackInfo& info); - - static void GetEnablePush( - Local property, - const PropertyCallbackInfo& info); - static void SetEnablePush( - Local property, - Local value, - const PropertyCallbackInfo& info); - - static void GetMaxConcurrentStreams( - Local property, - const PropertyCallbackInfo& info); - static void SetMaxConcurrentStreams( - Local property, - Local value, - const PropertyCallbackInfo& info); - - static void GetInitialWindowSize( - Local property, - const PropertyCallbackInfo& info); - static void SetInitialWindowSize( - Local property, - Local value, - const PropertyCallbackInfo& info); - - static void GetMaxFrameSize( - Local property, - const PropertyCallbackInfo& info); - static void SetMaxFrameSize( - Local property, - Local value, - const PropertyCallbackInfo& info); - - static void GetMaxHeaderListSize( - Local property, - const PropertyCallbackInfo& info); - static void SetMaxHeaderListSize( - Local property, - Local value, - const PropertyCallbackInfo& info); - - void CollectSettings(std::vector* entries) { - for (auto it = settings_.begin(); - it != settings_.end(); it++) { - entries->push_back({it->first, it->second}); - } - } - - size_t size() { - return settings_.size(); - } - - private: - void Set(int32_t id, uint32_t value) { - settings_[id] = value; - } - - void Find(int32_t id, const PropertyCallbackInfo& info) { - auto p = settings_.find(id); - if (p != settings_.end()) - info.GetReturnValue().Set(p->second); - } - - void FindBoolean(int32_t id, const PropertyCallbackInfo& info) { - auto p = settings_.find(id); - if (p != settings_.end()) - info.GetReturnValue().Set(p->second != 0); - } - - void Erase(int32_t id) { - settings_.erase(id); - } - - std::map settings_; -}; - class Http2Priority { public: Http2Priority(int32_t parent, @@ -751,11 +653,11 @@ class Http2Stream : public AsyncWrap, public StreamBase { class SessionIdler { public: - SessionIdler(Http2Session* session) : session_(session) {} + explicit SessionIdler(Http2Session* session) : session_(session) {} - ~SessionIdler() { - session_ = nullptr; - } + ~SessionIdler() { + session_ = nullptr; + } Http2Session* session() { return session_; @@ -765,7 +667,7 @@ class SessionIdler { return &idler_; } - uv_idle_t idler_; + uv_idle_t idler_; private: Http2Session* session_; }; From 704bd6e41025aa748aac1401b077e6eaa025ac68 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 10 Dec 2016 11:14:27 -0800 Subject: [PATCH 12/13] Remove Http2Headers class, simplify --- lib/internal/http2.js | 133 ++++++++++++++++++++++++------------------ src/node_http2.cc | 117 ++++++++++++++++--------------------- src/node_http2.h | 56 ------------------ 3 files changed, 127 insertions(+), 179 deletions(-) diff --git a/lib/internal/http2.js b/lib/internal/http2.js index e54609a7f5..8eb5a066cc 100644 --- a/lib/internal/http2.js +++ b/lib/internal/http2.js @@ -294,8 +294,9 @@ class Http2Stream extends Duplex { } get uid() { - if (this[kHandle]) - return this[kHandle].getUid(); + var handle = this[kHandle]; + if (handle) + return handle.getUid(); } set id(id) { @@ -312,22 +313,25 @@ class Http2Stream extends Duplex { get state() { var obj = {}; - if (this[kHandle]) - this[kHandle].getState(obj); + var handle = this[kHandle]; + if (handle) + handle.getState(obj); return obj; } setLocalWindowSize(size) { - if (this[kHandle]) { - this[kHandle].setLocalWindowSize(size); + var handle = this[kHandle]; + if (handle) { + handle.setLocalWindowSize(size); } else { this.once('handle', this.setLocalWindowSize.bind(this, size)); } } changeStreamPriority(parentId, priority, exclusive) { - if (this[kHandle]) { - this[kHandle].changeStreamPriority(parentId, priority, exclusive); + var handle = this[kHandle]; + if (handle) { + handle.changeStreamPriority(parentId, priority, exclusive); } else { this.once('handle', this.changeStreamPriority.bind(this, @@ -338,24 +342,27 @@ class Http2Stream extends Duplex { } respond(headers) { - if (this[kHandle]) { - this[kHandle].respond(headers); + var handle = this[kHandle]; + if (handle) { + handle.respond(headers); } else { this.once('handle', onHandleRespond(headers)); } } sendContinue() { - if (this[kHandle]) { - this[kHandle].sendContinue(); + var handle = this[kHandle]; + if (handle) { + handle.sendContinue(); } else { this.once('handle', this.sendContinue.bind(this)); } } sendPriority(parentId, priority, exclusive) { - if (this[kHandle]) { - this[kHandle].sendPriority(parentId, priority, exclusive); + var handle = this[kHandle]; + if (handle) { + handle.sendPriority(parentId, priority, exclusive); } else { this.once('handle', this.sendPriority.bind(this, @@ -366,24 +373,30 @@ class Http2Stream extends Duplex { } sendRstStream(code) { - if (this[kHandle]) { - this[kHandle].sendRstStream(code); + var handle = this[kHandle]; + if (handle) { + handle.sendRstStream(code); } else { this.once('handle', this.sendRstStream.bind(this, code)); } } sendPushPromise(headers) { - if (this[kHandle]) { - return this[kHandle].sendPushPromise(mapToHeaders(headers)); + headers = headers || {}; + if (typeof headers !== 'object') + throw new TypeError('headers must be an object'); + var handle = this[kHandle]; + if (handle) { + return handle.sendPushPromise(mapToHeaders(headers)); } else { this.once('handle', this.sendPushPromise.bind(this, headers)); } } addTrailer(name, value, noindex) { - if (this[kHandle]) { - this[kHandle].addTrailer(name, value, noindex); + var handle = this[kHandle]; + if (handle) { + handle.addTrailer(name, value, noindex); } else { this.once('handle', this.addTrailer.bind(this, name, value, noindex)); } @@ -418,15 +431,16 @@ class Http2Stream extends Duplex { } _write(data, encoding, cb) { - if (this[kHandle]) { + var handle = this[kHandle]; + if (handle) { unrefTimer(this); var req = new WriteWrap(); - req.handle = this[kHandle]; + req.handle = handle; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; var enc = data instanceof Buffer ? 'buffer' : encoding; - var err = createWriteReq(req, this[kHandle], data, enc); + var err = createWriteReq(req, handle, data, enc); if (err) throw util._errnoException(err, 'write', req.error); this._bytesDispatched += req.bytes; @@ -436,10 +450,11 @@ class Http2Stream extends Duplex { } _writev(data, cb) { - if (this[kHandle]) { + var handle = this[kHandle]; + if (handle) { unrefTimer(this); var req = new WriteWrap(); - req.handle = this[kHandle]; + req.handle = handle; req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; @@ -449,7 +464,7 @@ class Http2Stream extends Duplex { chunks[i * 2] = entry.chunk; chunks[i * 2 + 1] = entry.encoding; } - var err = this[kHandle].writev(req, chunks); + var err = handle.writev(req, chunks); if (err) throw util._errnoException(err, 'write', req.error); } else { @@ -458,8 +473,9 @@ class Http2Stream extends Duplex { } _read(n) { - if (this[kHandle]) { - this[kHandle].readStart(); + var handle = this[kHandle]; + if (handle) { + handle.readStart(); } else { this.once('handle', onHandleReadStart); } @@ -468,14 +484,16 @@ class Http2Stream extends Duplex { function afterShutdown() {} function onHandleFinish() { + var handle = this[kHandle]; var req = new ShutdownWrap(); req.oncomplete = afterShutdown; - req.handle = this[kHandle]; - this[kHandle].shutdown(req); + req.handle = handle; + handle.shutdown(req); } function onHandleReadStart() { - this[kHandle].readStart(); + var handle = this[kHandle]; + handle.readStart(); } function onHandleWrite(data, encoding, cb) { @@ -492,7 +510,8 @@ function onHandleWritev(chunks, cb) { function onHandleRespond(headers) { return function(headers) { - this[kHandle].respond(); + var handle = this[kHandle]; + handle.respond(); }; } @@ -509,8 +528,9 @@ class Http2Session extends EventEmitter { } reset() { - if (this[kHandle]) - this[kHandle].reset(); + var handle = this[kHandle]; + if (handle) + handle.reset(); } get _handle() { @@ -518,8 +538,9 @@ class Http2Session extends EventEmitter { } get uid() { - if (this[kHandle]) - return this[kHandle].getUid(); + var handle = this[kHandle]; + if (handle) + return handle.getUid(); } get type() { @@ -528,19 +549,22 @@ class Http2Session extends EventEmitter { get state() { var obj = {}; - if (this[kHandle]) - this[kHandle].getState(obj); + var handle = this[kHandle]; + if (handle) + handle.getState(obj); return obj; } setNextStreamID(id) { - if (this[kHandle]) - this[kHandle].setNextStreamID(id); + var handle = this[kHandle]; + if (handle) + handle.setNextStreamID(id); } setLocalWindowSize(size) { - if (this[kHandle]) - this[kHandle].setLocalWindowSize(size); + var handle = this[kHandle]; + if (handle) + handle.setLocalWindowSize(size); } getRemoteSettings() { @@ -583,16 +607,18 @@ class Http2Session extends EventEmitter { if (typeof callback !== 'function') throw new TypeError('callback must be a function'); - this[kHandle].startGracefulTerminate(); + var handle = this[kHandle]; + handle.startGracefulTerminate(); process.nextTick(() => { - this[kHandle].terminate(code || constants.NGHTTP2_NO_ERROR); + handle.terminate(code || constants.NGHTTP2_NO_ERROR); callback(); }); } request(headers) { - if (this[kHandle]) - return this[kHandle].request(headers); + var handle = this[kHandle]; + if (handle) + return handle.request(headers); } } @@ -1055,13 +1081,7 @@ function isIllegalConnectionSpecificHeader(name, value) { } } -// Converts an object into an http2.Http2Headers object. -// The Http2Headers object maintains an internal array -// of nghttp2_nv objects that contain a copy of the -// header value pairs as an std::vector. To avoid -// that vector having to copy and reallocate, we count -// the number of expected items up front (it's less -// expensive to count than it is to reallocate). +// Converts an object into an array of header entries. function mapToHeaders(map) { var keys = Object.keys(map); var size = keys.length; @@ -1070,17 +1090,18 @@ function mapToHeaders(map) { size += keys[i].length - 1; } } - var ret = new http2.Http2Headers(size); + var ret = Array(size); + var c = 0; for (i = 0; i < keys.length; i++) { var key = keys[i]; var value = map[key]; if (Array.isArray(value) && value.length > 0) { for (var k = 0; k < value.length; k++) { - ret.add(key, String(value[k])); + ret[c++] = [key, String(value[k])]; } } else { - ret.add(key, String(value)); + ret[c++] = [key, String(value)]; } } return ret; diff --git a/src/node_http2.cc b/src/node_http2.cc index 905db18c44..9077dde90d 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -32,7 +32,6 @@ using v8::Isolate; using v8::Local; using v8::Name; using v8::Object; -using v8::PropertyCallbackInfo; using v8::String; using v8::Value; @@ -70,45 +69,6 @@ Http2Options::Http2Options(Environment* env, Local options) { } #undef OPTIONS -// Http2Headers statics - -// Create a new Http2Headers object. The first argument is the -// number of headers expected in order to reserve the space. -void Http2Headers::New(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - new Http2Headers(env, args.This(), args[0]->IntegerValue()); -} - -void Http2Headers::Add(const FunctionCallbackInfo& args) { - Http2Headers* headers; - ASSIGN_OR_RETURN_UNWRAP(&headers, args.Holder()); - Environment* env = headers->env(); - Utf8Value key(env->isolate(), args[0]); - Utf8Value value(env->isolate(), args[1]); - bool noindex = args[2]->BooleanValue(); - headers->Add(*key, *value, key.length(), value.length(), noindex); -} - -void Http2Headers::Reserve(const FunctionCallbackInfo& args) { - Http2Headers* headers; - ASSIGN_OR_RETURN_UNWRAP(&headers, args.Holder()); - headers->Reserve(args[0]->IntegerValue()); -} - -void Http2Headers::Clear(const FunctionCallbackInfo& args) { - Http2Headers* headers; - ASSIGN_OR_RETURN_UNWRAP(&headers, args.Holder()); - headers->Clear(); -} - -void Http2Headers::GetSize(Local property, - const PropertyCallbackInfo& info) { - Http2Headers* headers; - ASSIGN_OR_RETURN_UNWRAP(&headers, info.Holder()); - Environment* env = headers->env(); - info.GetReturnValue().Set(Integer::New(env->isolate(), headers->Size())); -} - // Http2Stream Statics void Http2Stream::New(const FunctionCallbackInfo& args) { @@ -388,11 +348,12 @@ void Http2Stream::ChangeStreamPriority( } // Send a PUSH_PROMISE frame, then create and return the Http2Stream -// instance that is associated. The first argument is an Http2Headers +// instance that is associated. The first argument is an array of header entries // object instance used to pass along the PUSH_PROMISE headers. If // this Http2Stream instance is detached, then this is a non-op void Http2Stream::SendPushPromise(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); HandleScope scope(env->isolate()); Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); @@ -401,14 +362,35 @@ void Http2Stream::SendPushPromise(const FunctionCallbackInfo& args) { if (session->type_ == SESSION_TYPE_CLIENT) { return env->ThrowError("Client Http2Session instances cannot use push"); } - Http2Headers* headers; - THROW_AND_RETURN_UNLESS_HTTP2HEADERS(env, args[0]); - ASSIGN_OR_RETURN_UNWRAP(&headers, args[0].As()); + + CHECK(args[0]->IsArray()); + std::vector headers_list; + Local headers = args[0].As(); + for (size_t n = 0; n < headers->Length(); n++) { + Local item = headers->Get(n); + if (item->IsArray()) { + Local header = item.As(); + Utf8Value key(isolate, header->Get(0)); + Utf8Value value(isolate, header->Get(1)); + const Http2Header* entry = + new Http2Header(*key, *value, + key.length(), + value.length(), + header->Get(2)->BooleanValue()); + if (strncmp(*key, ":", 1) == 0) { + headers_list.insert(headers_list.begin(), *entry); + } else { + headers_list.push_back(*entry); + } + } + } + + int32_t rv = nghttp2_submit_push_promise(**session, NGHTTP2_FLAG_NONE, stream->id(), - **headers, headers->Size(), + &headers_list[0], headers_list.size(), stream); session->EmitErrorIfFail(rv); @@ -1144,7 +1126,7 @@ void Http2Session::GracefulTerminate(const FunctionCallbackInfo& args) { } // Initiate sending a request. Request headers must be passed as an -// argument in the form of an Http2Headers object. This will result +// argument in the form of an array of header entries. This will result // in sending an initial HEADERS frame (or multiple), zero or more // DATA frames, and zero or more trailing HEADERS frames. void Http2Session::Request(const FunctionCallbackInfo& args) { @@ -1159,9 +1141,27 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { Local obj = env->http2stream_object()->Clone(); Http2Stream* stream = new Http2Stream(env, scope.Escape(obj)); - Http2Headers* headers; - THROW_AND_RETURN_UNLESS_HTTP2HEADERS(env, args[0]); - ASSIGN_OR_RETURN_UNWRAP(&headers, args[0].As()); + CHECK(args[0]->IsArray()); + std::vector headers_list; + Local headers = args[0].As(); + for (size_t n = 0; n < headers->Length(); n++) { + Local item = headers->Get(n); + if (item->IsArray()) { + Local header = item.As(); + Utf8Value key(isolate, header->Get(0)); + Utf8Value value(isolate, header->Get(1)); + const Http2Header* entry = + new Http2Header(*key, *value, + key.length(), + value.length(), + header->Get(2)->BooleanValue()); + if (strncmp(*key, ":", 1) == 0) { + headers_list.insert(headers_list.begin(), *entry); + } else { + headers_list.push_back(*entry); + } + } + } bool nodata = args[1]->BooleanValue(); nghttp2_data_provider* provider = nodata ? nullptr : stream->provider(); @@ -1169,7 +1169,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { const nghttp2_priority_spec* pri = NULL; int32_t rv = nghttp2_submit_request(**session, pri, - **headers, headers->Size(), + &headers_list[0], headers_list.size(), provider, stream); session->EmitErrorIfFail(rv); @@ -1266,9 +1266,6 @@ void Initialize(Local target, // Method to fetch the nghttp2 string description of an nghttp2 error code env->SetMethod(target, "nghttp2ErrorString", HttpErrorString); - Local http2HeadersClassName = - String::NewFromUtf8(isolate, "Http2Headers", - v8::NewStringType::kInternalized).ToLocalChecked(); Local http2SessionClassName = String::NewFromUtf8(isolate, "Http2Session", v8::NewStringType::kInternalized).ToLocalChecked(); @@ -1305,20 +1302,6 @@ void Initialize(Local target, stream->GetFunction()->NewInstance(env->context()).ToLocalChecked()); target->Set(http2StreamClassName, stream->GetFunction()); - // Http2Headers Template - Local headers = - env->NewFunctionTemplate(Http2Headers::New); - headers->InstanceTemplate()->SetInternalFieldCount(1); - headers->SetClassName(http2HeadersClassName); - env->SetAccessor(headers, "size", Http2Headers::GetSize); - env->SetProtoMethod(headers, "add", Http2Headers::Add); - env->SetProtoMethod(headers, "clear", Http2Headers::Clear); - env->SetProtoMethod(headers, "reserve", Http2Headers::Reserve); - env->set_http2headers_constructor_template(headers); - target->Set(context, - http2HeadersClassName, - headers->GetFunction()).FromJust(); - // Http2Session Template Local t = env->NewFunctionTemplate(Http2Session::New); diff --git a/src/node_http2.h b/src/node_http2.h index 48c6f22e9f..4bc441f0ed 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -35,7 +35,6 @@ using v8::Map; using v8::Name; using v8::Object; using v8::Persistent; -using v8::PropertyCallbackInfo; using v8::String; using v8::Value; @@ -135,9 +134,6 @@ enum http2_data_flags { return env->ThrowTypeError("argument must be an " #name " instance"); \ } while (0) -#define THROW_AND_RETURN_UNLESS_HTTP2HEADERS(env, obj) \ - THROW_AND_RETURN_UNLESS_(http2headers, "Http2Headers", env, obj); - #define THROW_AND_RETURN_UNLESS_HTTP2SETTINGS(env, obj) \ THROW_AND_RETURN_UNLESS_(http2settings, "Http2Settings", env, obj); @@ -342,58 +338,6 @@ class Http2Header : public nghttp2_nv { } }; -class Http2Headers : public BaseObject { - public: - Http2Headers(Environment* env, - Local wrap, - int reserve) : - BaseObject(env, wrap) { - MakeWeak(this); - entries_.reserve(reserve); - } - - ~Http2Headers() {} - - static void New(const FunctionCallbackInfo& args); - static void Add(const FunctionCallbackInfo& args); - static void Clear(const FunctionCallbackInfo& args); - static void Reserve(const FunctionCallbackInfo& args); - static void GetSize(Local property, - const PropertyCallbackInfo& info); - - size_t Size() { - return entries_.size(); - } - - nghttp2_nv* operator*() { - return &entries_[0]; - } - - private: - void Add(const char* name, - const char* value, - size_t nlen, - size_t vlen, - bool noindex = false) { - uint8_t flags = NGHTTP2_NV_FLAG_NONE; - if (noindex) - flags |= NGHTTP2_NV_FLAG_NO_INDEX; - const Http2Header* header = - new Http2Header(name, value, nlen, vlen, noindex); - entries_.push_back(*header); - } - - void Reserve(int inc) { - entries_.reserve(entries_.size() + inc); - } - - void Clear() { - entries_.clear(); - } - - std::vector entries_; -}; - class Http2Stream : public AsyncWrap, public StreamBase { public: // Get a stored Http2Stream instance from the nghttp2_session, if one exists From 757982b8e6dff19f3493b5d7867764d3e698dd6d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 12 Dec 2016 12:52:03 -0800 Subject: [PATCH 13/13] Additional minor refactor --- src/node_http2.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 9077dde90d..9582d3421f 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2,6 +2,7 @@ #include "node_buffer.h" #include "nghttp2/nghttp2.h" #include "node_http2.h" +#include "node_http2_core.h" #include "stream_base.h" #include "stream_base-inl.h" @@ -436,9 +437,6 @@ int Http2Stream::DoWrite(WriteWrap* w, size_t count, uv_stream_t* send_handle) { // Buffer the data for the Data Provider - - session()->SendIfNecessary(); - if (w->object()->Has(FIXED_ONE_BYTE_STRING(env()->isolate(), "ending"))) writable_ = false; @@ -456,6 +454,8 @@ int Http2Stream::DoWrite(WriteWrap* w, w->Dispatched(); w->Done(0); + session()->SendIfNecessary(); + return 0; }