src,http2: DRY header/trailer handling code up

Remove duplicate code through minor refactoring. PR-URL: #14688 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Aug 14, 2017 · dd6444d · dd6444d
1 parent af85b6e
commit dd6444d
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 63 deletions.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -104,47 +104,6 @@ Http2Options::Http2Options(Environment* env) {
   }
 }
 
-inline void CopyHeaders(Isolate* isolate,
-                        Local<Context> context,
-                        MaybeStackBuffer<nghttp2_nv>* list,
-                        Local<Array> headers) {
-  Local<Value> item;
-  Local<Array> header;
-
-  for (size_t n = 0; n < headers->Length(); n++) {
-    item = headers->Get(context, n).ToLocalChecked();
-    header = item.As<Array>();
-    Local<Value> key = header->Get(context, 0).ToLocalChecked();
-    Local<Value> value = header->Get(context, 1).ToLocalChecked();
-    CHECK(key->IsString());
-    CHECK(value->IsString());
-    size_t keylen = StringBytes::StorageSize(isolate, key, ASCII);
-    size_t valuelen = StringBytes::StorageSize(isolate, value, ASCII);
-    nghttp2_nv& nv = (*list)[n];
-    nv.flags = NGHTTP2_NV_FLAG_NONE;
-    Local<Value> flag = header->Get(context, 2).ToLocalChecked();
-    if (flag->BooleanValue(context).ToChecked())
-      nv.flags |= NGHTTP2_NV_FLAG_NO_INDEX;
-    nv.name = Malloc<uint8_t>(keylen);
-    nv.value = Malloc<uint8_t>(valuelen);
-    nv.namelen =
-        StringBytes::Write(isolate,
-                           reinterpret_cast<char*>(nv.name),
-                           keylen, key, ASCII);
-    nv.valuelen =
-        StringBytes::Write(isolate,
-                           reinterpret_cast<char*>(nv.value),
-                           valuelen, value, ASCII);
-  }
-}
-
-inline void FreeHeaders(MaybeStackBuffer<nghttp2_nv>* list) {
-  for (size_t n = 0; n < list->length(); n++) {
-    free((*list)[n].name);
-    free((*list)[n].value);
-  }
-}
-
 void Http2Session::OnFreeSession() {
   ::delete this;
 }
@@ -860,7 +819,7 @@ void Http2Session::Send(uv_buf_t* buf, size_t length) {
 }
 
 void Http2Session::OnTrailers(Nghttp2Stream* stream,
-                              MaybeStackBuffer<nghttp2_nv>* trailers) {
+                              const SubmitTrailers& submit_trailers) {
   DEBUG_HTTP2("Http2Session: prompting for trailers on stream %d\n",
               stream->id());
   Local<Context> context = env()->context();
@@ -879,8 +838,8 @@ void Http2Session::OnTrailers(Nghttp2Stream* stream,
       if (ret->IsArray()) {
         Local<Array> headers = ret.As<Array>();
         if (headers->Length() > 0) {
-          trailers->AllocateSufficientStorage(headers->Length());
-          CopyHeaders(isolate, context, trailers, headers);
+          Headers trailers(isolate, context, headers);
+          submit_trailers.Submit(*trailers, trailers.length());
         }
       }
     }

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -387,7 +387,7 @@ class Http2Session : public AsyncWrap,
                 size_t length) override;
   void OnFrameError(int32_t id, uint8_t type, int error_code) override;
   void OnTrailers(Nghttp2Stream* stream,
-                  MaybeStackBuffer<nghttp2_nv>* trailers) override;
+                  const SubmitTrailers& submit_trailers) override;
   void AllocateSend(size_t recommended, uv_buf_t* buf) override;
 
   int DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count,

diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h
@@ -605,6 +605,12 @@ Nghttp2Session::Callbacks::~Callbacks() {
   nghttp2_session_callbacks_del(callbacks);
 }
 
+Nghttp2Session::SubmitTrailers::SubmitTrailers(
+    Nghttp2Session* handle,
+    Nghttp2Stream* stream,
+    uint32_t* flags)
+  : handle_(handle), stream_(stream), flags_(flags) { }
+
 }  // namespace http2
 }  // namespace node
 

diff --git a/src/node_http2_core.cc b/src/node_http2_core.cc
@@ -172,25 +172,24 @@ void Nghttp2Session::GetTrailers(nghttp2_session* session,
     // any trailing headers that are to be sent. This is the only opportunity
     // we have to make this check. If there are trailers, then the
     // NGHTTP2_DATA_FLAG_NO_END_STREAM flag must be set.
-    MaybeStackBuffer<nghttp2_nv> trailers;
-    handle->OnTrailers(stream, &trailers);
-    if (trailers.length() > 0) {
-      DEBUG_HTTP2("Nghttp2Session %d: sending trailers for stream %d, "
-                  "count: %d\n", handle->session_type_, stream->id(),
-                  trailers.length());
-      *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
-      nghttp2_submit_trailer(session,
-                             stream->id(),
-                             *trailers,
-                             trailers.length());
-    }
-    for (size_t n = 0; n < trailers.length(); n++) {
-      free(trailers[n].name);
-      free(trailers[n].value);
-    }
+    SubmitTrailers submit_trailers { handle, stream, flags };
+    handle->OnTrailers(stream, submit_trailers);
   }
 }
 
+void Nghttp2Session::SubmitTrailers::Submit(nghttp2_nv* trailers,
+                                            size_t length) const {
+  if (length == 0) return;
+  DEBUG_HTTP2("Nghttp2Session %d: sending trailers for stream %d, "
+              "count: %d\n", handle_->session_type_, stream_->id(),
+              length);
+  *flags_ |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
+  nghttp2_submit_trailer(handle_->session_,
+                         stream_->id(),
+                         trailers,
+                         length);
+}
+
 // Called by nghttp2 to collect the data while a file response is sent.
 // The buf is the DATA frame buffer that needs to be filled with at most
 // length bytes. flags is used to control what nghttp2 does next.

diff --git a/src/node_http2_core.h b/src/node_http2_core.h
@@ -166,13 +166,30 @@ class Nghttp2Session {
                             int error_code) {}
   virtual ssize_t GetPadding(size_t frameLength,
                              size_t maxFrameLength) { return 0; }
-  virtual void OnTrailers(Nghttp2Stream* stream,
-                          MaybeStackBuffer<nghttp2_nv>* nva) {}
   virtual void OnFreeSession() {}
   virtual void AllocateSend(size_t suggested_size, uv_buf_t* buf) = 0;
 
   virtual bool HasGetPaddingCallback() { return false; }
 
+  class SubmitTrailers {
+   public:
+    void Submit(nghttp2_nv* trailers, size_t length) const;
+
+   private:
+    inline SubmitTrailers(Nghttp2Session* handle,
+                          Nghttp2Stream* stream,
+                          uint32_t* flags);
+
+    Nghttp2Session* const handle_;
+    Nghttp2Stream* const stream_;
+    uint32_t* const flags_;
+
+    friend class Nghttp2Session;
+  };
+
+  virtual void OnTrailers(Nghttp2Stream* stream,
+                          const SubmitTrailers& submit_trailers) {}
+
  private:
   inline void SendPendingData();
   inline void HandleHeadersFrame(const nghttp2_frame* frame);