src: pass along errors from http2 object creation

[This backport applied to v10.x cleanly.] Backport-PR-URL: #29123 PR-URL: #25822 Reviewed-By: Gireesh Punathil <[email protected]>
nodejs · Aug 15, 2019 · a0a14c8 · a0a14c8
1 parent 00f153d
commit a0a14c8
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 62 deletions.
diff --git 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<Object> 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
@@ -916,13 +907,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);
   }
@@ -1789,7 +1781,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;
 }
 
@@ -1875,20 +1867,30 @@ void Http2Session::Consume(Local<External> 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<Object> 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<Object> 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();
 
@@ -2132,7 +2134,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;
 }
@@ -2354,7 +2356,14 @@ void HttpErrorString(const FunctionCallbackInfo<Value>& args) {
 // output for an HTTP2-Settings header field.
 void PackSettings(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  Http2Session::Http2Settings settings(env);
+  // TODO(addaleax): We should not be creating a full AsyncWrap for this.
+  Local<Object> obj;
+  if (!env->http2settings_constructor_template()
+           ->NewInstance(env->context())
+           .ToLocal(&obj)) {
+    return;
+  }
+  Http2Session::Http2Settings settings(env, nullptr, obj);
   args.GetReturnValue().Set(settings.Pack());
 }
 
@@ -2483,7 +2492,7 @@ void Http2Session::Request(const FunctionCallbackInfo<Value>& 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);
   }
@@ -2656,7 +2665,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo<Value>& 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);
   }
@@ -2792,9 +2801,15 @@ void Http2Session::Ping(const FunctionCallbackInfo<Value>& args) {
     CHECK_EQ(Buffer::Length(args[0]), 8);
   }
 
-  Http2Session::Http2Ping* ping = new Http2Ping(session);
-  Local<Object> obj = ping->object();
-  obj->Set(env->context(), env->ondone_string(), args[1]).FromJust();
+  Local<Object> 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
@@ -2818,10 +2833,17 @@ void Http2Session::Settings(const FunctionCallbackInfo<Value>& args) {
   Http2Session* session;
   ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
 
-  Http2Session::Http2Settings* settings = new Http2Settings(session);
-  Local<Object> obj = settings->object();
-  obj->Set(env->context(), env->ondone_string(), args[0]).FromJust();
+  Local<Object> 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);
@@ -2868,15 +2890,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<Object> 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
@@ -444,10 +444,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*();
@@ -604,6 +605,12 @@ class Http2Stream : public AsyncWrap,
   Statistics statistics_ = {};
 
  private:
+  Http2Stream(Http2Session* session,
+              v8::Local<v8::Object> 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)
@@ -1080,7 +1087,7 @@ class Http2StreamPerformanceEntry : public PerformanceEntry {
 
 class Http2Session::Http2Ping : public AsyncWrap {
  public:
-  explicit Http2Ping(Http2Session* session);
+  explicit Http2Ping(Http2Session* session, v8::Local<v8::Object> obj);
 
   void MemoryInfo(MemoryTracker* tracker) const override {
     tracker->TrackField("session", session_);
@@ -1104,8 +1111,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<v8::Object> obj,
+                uint64_t start_time = uv_hrtime());
 
   void MemoryInfo(MemoryTracker* tracker) const override {
     tracker->TrackField("session", session_);
@@ -1129,7 +1138,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_;