From 0672c24dc313f8fd53bc628683ba10ec46f98c08 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 18:43:05 +0100 Subject: [PATCH] src: pass along errors from http2 object creation PR-URL: https://github.com/nodejs/node/pull/25822 Reviewed-By: Gireesh Punathil --- src/node_http2.cc | 125 ++++++++++++++++++++++++++-------------------- src/node_http2.h | 24 ++++++--- 2 files changed, 87 insertions(+), 62 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7904c680b89369..0d349df060a5b4 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -228,27 +228,18 @@ void Http2Session::Http2Settings::Init() { count_ = n; } -Http2Session::Http2Settings::Http2Settings(Environment* env, - Http2Session* session, uint64_t start_time) - : AsyncWrap(env, - env->http2settings_constructor_template() - ->NewInstance(env->context()) - .ToLocalChecked(), - PROVIDER_HTTP2SETTINGS), - session_(session), - startTime_(start_time) { - Init(); -} - - -Http2Session::Http2Settings::Http2Settings(Environment* env) - : Http2Settings(env, nullptr, 0) {} - // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray // that is shared with the JavaScript side. -Http2Session::Http2Settings::Http2Settings(Http2Session* session) - : Http2Settings(session->env(), session, uv_hrtime()) {} +Http2Session::Http2Settings::Http2Settings(Environment* env, + Http2Session* session, + Local obj, + uint64_t start_time) + : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), + session_(session), + startTime_(start_time) { + Init(); +} // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded @@ -918,13 +909,14 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, // The common case is that we're creating a new stream. The less likely // case is that we're receiving a set of trailers if (LIKELY(stream == nullptr)) { - if (UNLIKELY(!session->CanAddStream())) { + if (UNLIKELY(!session->CanAddStream() || + Http2Stream::New(session, id, frame->headers.cat) == + nullptr)) { // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - new Http2Stream(session, id, frame->headers.cat); } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } @@ -1771,7 +1763,7 @@ Http2Stream* Http2Session::SubmitRequest( *ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); if (LIKELY(*ret > 0)) - stream = new Http2Stream(this, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(this, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -1857,20 +1849,30 @@ void Http2Session::Consume(Local external) { Debug(this, "i/o stream consumed"); } - -Http2Stream::Http2Stream( - Http2Session* session, - int32_t id, - nghttp2_headers_category category, - int options) : AsyncWrap(session->env(), - session->env()->http2stream_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2STREAM), - StreamBase(session->env()), - session_(session), - id_(id), - current_headers_category_(category) { +Http2Stream* Http2Stream::New(Http2Session* session, + int32_t id, + nghttp2_headers_category category, + int options) { + Local obj; + if (!session->env() + ->http2stream_constructor_template() + ->NewInstance(session->env()->context()) + .ToLocal(&obj)) { + return nullptr; + } + return new Http2Stream(session, obj, id, category, options); +} + +Http2Stream::Http2Stream(Http2Session* session, + Local obj, + int32_t id, + nghttp2_headers_category category, + int options) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2STREAM), + StreamBase(session->env()), + session_(session), + id_(id), + current_headers_category_(category) { MakeWeak(); statistics_.start_time = uv_hrtime(); @@ -2113,7 +2115,7 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); Http2Stream* stream = nullptr; if (*ret > 0) - stream = new Http2Stream(session_, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -2335,7 +2337,14 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Http2Session::Http2Settings settings(env); + // TODO(addaleax): We should not be creating a full AsyncWrap for this. + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + Http2Session::Http2Settings settings(env, nullptr, obj); args.GetReturnValue().Set(settings.Pack()); } @@ -2464,7 +2473,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { session->Http2Session::SubmitRequest(*priority, *list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(session, "could not submit request: %s", nghttp2_strerror(ret)); return args.GetReturnValue().Set(ret); } @@ -2637,7 +2646,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { int32_t ret = 0; Http2Stream* stream = parent->SubmitPushPromise(*list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(parent, "failed to create push stream: %d", ret); return args.GetReturnValue().Set(ret); } @@ -2773,9 +2782,15 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { CHECK_EQ(Buffer::Length(args[0]), 8); } - Http2Session::Http2Ping* ping = new Http2Ping(session); - Local obj = ping->object(); - obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); + Local obj; + if (!env->http2ping_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) + return; + Http2Session::Http2Ping* ping = new Http2Ping(session, obj); // To prevent abuse, we strictly limit the number of unacknowledged PING // frames that may be sent at any given time. This is configurable in the @@ -2799,10 +2814,17 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - Http2Session::Http2Settings* settings = new Http2Settings(session); - Local obj = settings->object(); - obj->Set(env->context(), env->ondone_string(), args[0]).FromJust(); + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) + return; + Http2Session::Http2Settings* settings = + new Http2Settings(session->env(), session, obj, 0); if (!session->AddSettings(settings)) { settings->Done(false); return args.GetReturnValue().Set(false); @@ -2849,15 +2871,10 @@ bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) { return true; } -Http2Session::Http2Ping::Http2Ping( - Http2Session* session) - : AsyncWrap(session->env(), - session->env()->http2ping_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2PING), - session_(session), - startTime_(uv_hrtime()) { } +Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), + session_(session), + startTime_(uv_hrtime()) {} void Http2Session::Http2Ping::Send(uint8_t* payload) { uint8_t data[8]; diff --git a/src/node_http2.h b/src/node_http2.h index e6953f6fc19802..fb90e3ed85111c 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -451,10 +451,11 @@ class Http2StreamListener : public StreamListener { class Http2Stream : public AsyncWrap, public StreamBase { public: - Http2Stream(Http2Session* session, - int32_t id, - nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - int options = 0); + static Http2Stream* New( + Http2Session* session, + int32_t id, + nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, + int options = 0); ~Http2Stream() override; nghttp2_stream* operator*(); @@ -611,6 +612,12 @@ class Http2Stream : public AsyncWrap, Statistics statistics_ = {}; private: + Http2Stream(Http2Session* session, + v8::Local obj, + int32_t id, + nghttp2_headers_category category, + int options); + Http2Session* session_ = nullptr; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) @@ -1076,7 +1083,7 @@ class Http2StreamPerformanceEntry : public PerformanceEntry { class Http2Session::Http2Ping : public AsyncWrap { public: - explicit Http2Ping(Http2Session* session); + explicit Http2Ping(Http2Session* session, v8::Local obj); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1100,8 +1107,10 @@ class Http2Session::Http2Ping : public AsyncWrap { // structs. class Http2Session::Http2Settings : public AsyncWrap { public: - explicit Http2Settings(Environment* env); - explicit Http2Settings(Http2Session* session); + Http2Settings(Environment* env, + Http2Session* session, + v8::Local obj, + uint64_t start_time = uv_hrtime()); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1125,7 +1134,6 @@ class Http2Session::Http2Settings : public AsyncWrap { get_setting fn); private: - Http2Settings(Environment* env, Http2Session* session, uint64_t start_time); void Init(); Http2Session* session_; uint64_t startTime_;