http2: multiple smaller code cleanups

jasnell · evanlucas · commit 8dfd5a515aae · 2017-11-13T08:00:11.000-06:00
* Add Http2Priority utility class * Reduces some code duplication * Other small minor cleanups PR-URL: #16764 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -186,6 +186,18 @@ inline void Http2Settings::RefreshDefaults(Environment* env) {
     (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE);
 }
 
+Http2Priority::Http2Priority(Environment* env,
+                             Local<Value> parent,
+                             Local<Value> weight,
+                             Local<Value> exclusive) {
+  Local<Context> context = env->context();
+  int32_t parent_ = parent->Int32Value(context).ToChecked();
+  int32_t weight_ = weight->Int32Value(context).ToChecked();
+  bool exclusive_ = exclusive->BooleanValue(context).ToChecked();
+  DEBUG_HTTP2("Http2Priority: parent: %d, weight: %d, exclusive: %d\n",
+              parent_, weight_, exclusive_);
+  nghttp2_priority_spec_init(&spec, parent_, weight_, exclusive_ ? 1 : 0);
+}
 
 Http2Session::Http2Session(Environment* env,
                            Local<Object> wrap,
@@ -267,12 +279,8 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
   buffer[PADDING_BUF_RETURN_VALUE] = frameLen;
   MakeCallback(env()->ongetpadding_string(), 0, nullptr);
   uint32_t retval = buffer[PADDING_BUF_RETURN_VALUE];
-  retval = retval <= maxPayloadLen ? retval : maxPayloadLen;
-  retval = retval >= frameLen ? retval : frameLen;
-#if defined(DEBUG) && DEBUG
-  CHECK_GE(retval, frameLen);
-  CHECK_LE(retval, maxPayloadLen);
-#endif
+  retval = std::min(retval, static_cast<uint32_t>(maxPayloadLen));
+  retval = std::max(retval, static_cast<uint32_t>(frameLen));
   return retval;
 }
 
@@ -454,30 +462,18 @@ void Http2Session::SubmitPriority(const FunctionCallbackInfo<Value>& args) {
   ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
   Local<Context> context = env->context();
 
-  nghttp2_priority_spec spec;
   int32_t id = args[0]->Int32Value(context).ToChecked();
-  int32_t parent = args[1]->Int32Value(context).ToChecked();
-  int32_t weight = args[2]->Int32Value(context).ToChecked();
-  bool exclusive = args[3]->BooleanValue(context).ToChecked();
+  Http2Priority priority(env, args[1], args[2], args[3]);
   bool silent = args[4]->BooleanValue(context).ToChecked();
-  DEBUG_HTTP2("Http2Session: submitting priority for stream %d: "
-              "parent: %d, weight: %d, exclusive: %d, silent: %d\n",
-              id, parent, weight, exclusive, silent);
-
-#if defined(DEBUG) && DEBUG
-  CHECK_GT(id, 0);
-  CHECK_GE(parent, 0);
-  CHECK_GE(weight, 0);
-#endif
+  DEBUG_HTTP2("Http2Session: submitting priority for stream %d", id);
 
   Nghttp2Stream* stream;
   if (!(stream = session->FindStream(id))) {
     // invalid stream
     return args.GetReturnValue().Set(NGHTTP2_ERR_INVALID_STREAM_ID);
   }
-  nghttp2_priority_spec_init(&spec, parent, weight, exclusive ? 1 : 0);
 
-  args.GetReturnValue().Set(stream->SubmitPriority(&spec, silent));
+  args.GetReturnValue().Set(stream->SubmitPriority(*priority, silent));
 }
 
 void Http2Session::SubmitSettings(const FunctionCallbackInfo<Value>& args) {
@@ -533,20 +529,14 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo<Value>& args) {
 
   Local<Array> headers = args[0].As<Array>();
   int options = args[1]->IntegerValue(context).ToChecked();
-  int32_t parent = args[2]->Int32Value(context).ToChecked();
-  int32_t weight = args[3]->Int32Value(context).ToChecked();
-  bool exclusive = args[4]->BooleanValue(context).ToChecked();
-
-  DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d, "
-              "parent: %d, weight: %d, exclusive: %d\n", headers->Length(),
-              options, parent, weight, exclusive);
+  Http2Priority priority(env, args[2], args[3], args[4]);
 
-  nghttp2_priority_spec prispec;
-  nghttp2_priority_spec_init(&prispec, parent, weight, exclusive ? 1 : 0);
+  DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d\n",
+              headers->Length(), options);
 
   Headers list(isolate, context, headers);
 
-  int32_t ret = session->Nghttp2Session::SubmitRequest(&prispec,
+  int32_t ret = session->Nghttp2Session::SubmitRequest(*priority,
                                                        *list, list.length(),
                                                        nullptr, options);
   DEBUG_HTTP2("Http2Session: request submitted, response: %d\n", ret);
diff --git a/src/node_http2.h b/src/node_http2.h
@@ -368,6 +368,20 @@ class Http2Settings {
   MaybeStackBuffer<nghttp2_settings_entry, IDX_SETTINGS_COUNT> entries_;
 };
 
+class Http2Priority {
+ public:
+  Http2Priority(Environment* env,
+                Local<Value> parent,
+                Local<Value> weight,
+                Local<Value> exclusive);
+
+  nghttp2_priority_spec* operator*() {
+    return &spec;
+  }
+ private:
+  nghttp2_priority_spec spec;
+};
+
 class Http2Session : public AsyncWrap,
                      public StreamBase,
                      public Nghttp2Session {
diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h
@@ -22,18 +22,21 @@ inline int Nghttp2Session::OnNghttpError(nghttp2_session* session,
 }
 #endif
 
+inline int32_t GetFrameID(const nghttp2_frame* frame) {
+  // If this is a push promise, we want to grab the id of the promised stream
+  return (frame->hd.type == NGHTTP2_PUSH_PROMISE) ?
+      frame->push_promise.promised_stream_id :
+      frame->hd.stream_id;
+}
+
 // nghttp2 calls this at the beginning a new HEADERS or PUSH_PROMISE frame.
 // We use it to ensure that an Nghttp2Stream instance is allocated to store
 // the state.
 inline int Nghttp2Session::OnBeginHeadersCallback(nghttp2_session* session,
                                                   const nghttp2_frame* frame,
                                                   void* user_data) {
   Nghttp2Session* handle = static_cast<Nghttp2Session*>(user_data);
-  // If this is a push promise frame, we want to grab the handle of
-  // the promised stream.
-  int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ?
-      frame->push_promise.promised_stream_id :
-      frame->hd.stream_id;
+  int32_t id = GetFrameID(frame);
   DEBUG_HTTP2("Nghttp2Session %s: beginning headers for stream %d\n",
               handle->TypeName(), id);
 
@@ -79,11 +82,7 @@ inline int Nghttp2Session::OnHeaderCallback(nghttp2_session* session,
                                             uint8_t flags,
                                             void* user_data) {
   Nghttp2Session* handle = static_cast<Nghttp2Session*>(user_data);
-  // If this is a push promise frame, we want to grab the handle of
-  // the promised stream.
-  int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ?
-      frame->push_promise.promised_stream_id :
-      frame->hd.stream_id;
+  int32_t id = GetFrameID(frame);
   Nghttp2Stream* stream = handle->FindStream(id);
   if (!stream->AddHeader(name, value, flags)) {
     // This will only happen if the connected peer sends us more
@@ -432,7 +431,7 @@ inline void Nghttp2Stream::FlushDataChunks() {
 // see if the END_STREAM flag is set, and will flush the queued data chunks
 // to JS if the stream is flowing
 inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) {
-  int32_t id = frame->hd.stream_id;
+  int32_t id = GetFrameID(frame);
   DEBUG_HTTP2("Nghttp2Session %s: handling data frame for stream %d\n",
               TypeName(), id);
   Nghttp2Stream* stream = this->FindStream(id);
@@ -450,8 +449,7 @@ inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) {
 // The headers are collected as the frame is being processed and sent out
 // to the JS side only when the frame is fully processed.
 inline void Nghttp2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
-  int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ?
-    frame->push_promise.promised_stream_id : frame->hd.stream_id;
+  int32_t id = GetFrameID(frame);
   DEBUG_HTTP2("Nghttp2Session %s: handling headers frame for stream %d\n",
               TypeName(), id);
   Nghttp2Stream* stream = FindStream(id);
@@ -469,7 +467,7 @@ inline void Nghttp2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
 // Notifies the JS layer that a PRIORITY frame has been received
 inline void Nghttp2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
   nghttp2_priority priority_frame = frame->priority;
-  int32_t id = frame->hd.stream_id;
+  int32_t id = GetFrameID(frame);
   DEBUG_HTTP2("Nghttp2Session %s: handling priority frame for stream %d\n",
               TypeName(), id);
 
@@ -571,41 +569,24 @@ inline int Nghttp2Session::Init(const nghttp2_session_type type,
   session_type_ = type;
   DEBUG_HTTP2("Nghttp2Session %s: initializing session\n", TypeName());
   destroying_ = false;
-  int ret = 0;
 
   max_header_pairs_ = maxHeaderPairs;
 
   nghttp2_session_callbacks* callbacks
       = callback_struct_saved[HasGetPaddingCallback() ? 1 : 0].callbacks;
 
-  nghttp2_option* opts;
-  if (options != nullptr) {
-    opts = options;
-  } else {
-    nghttp2_option_new(&opts);
-  }
+  CHECK_NE(options, nullptr);
 
-  switch (type) {
-    case NGHTTP2_SESSION_SERVER:
-      ret = nghttp2_session_server_new3(&session_,
-                                        callbacks,
-                                        this,
-                                        opts,
-                                        mem);
-      break;
-    case NGHTTP2_SESSION_CLIENT:
-      ret = nghttp2_session_client_new3(&session_,
-                                        callbacks,
-                                        this,
-                                        opts,
-                                        mem);
-      break;
-  }
-  if (opts != options) {
-    nghttp2_option_del(opts);
-  }
+  typedef int (*init_fn)(nghttp2_session** session,
+                         const nghttp2_session_callbacks* callbacks,
+                         void* user_data,
+                         const nghttp2_option* options,
+                         nghttp2_mem* mem);
+  init_fn fn = type == NGHTTP2_SESSION_SERVER ?
+      nghttp2_session_server_new3 :
+      nghttp2_session_client_new3;
 
-  return ret;
+  return fn(&session_, callbacks, this, options, mem);
 }
 
 inline void Nghttp2Session::MarkDestroying() {
