From dd6444d401676f5d295a5f7a4ea46b375beb37ce Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 8 Aug 2017 15:32:22 +0200 Subject: [PATCH] src,http2: DRY header/trailer handling code up Remove duplicate code through minor refactoring. PR-URL: https://github.com/nodejs/node/pull/14688 Reviewed-By: Daniel Bevenius Reviewed-By: Colin Ihrig Reviewed-By: James Snell Reviewed-By: Ben Noordhuis --- src/node_http2.cc | 47 +++------------------------------------ src/node_http2.h | 2 +- src/node_http2_core-inl.h | 6 +++++ src/node_http2_core.cc | 31 +++++++++++++------------- src/node_http2_core.h | 21 +++++++++++++++-- 5 files changed, 44 insertions(+), 63 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 2b873595a89b9d..2155061ef51feb 100755 --- 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, - MaybeStackBuffer* list, - Local headers) { - Local item; - Local header; - - for (size_t n = 0; n < headers->Length(); n++) { - item = headers->Get(context, n).ToLocalChecked(); - header = item.As(); - Local key = header->Get(context, 0).ToLocalChecked(); - Local 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 flag = header->Get(context, 2).ToLocalChecked(); - if (flag->BooleanValue(context).ToChecked()) - nv.flags |= NGHTTP2_NV_FLAG_NO_INDEX; - nv.name = Malloc(keylen); - nv.value = Malloc(valuelen); - nv.namelen = - StringBytes::Write(isolate, - reinterpret_cast(nv.name), - keylen, key, ASCII); - nv.valuelen = - StringBytes::Write(isolate, - reinterpret_cast(nv.value), - valuelen, value, ASCII); - } -} - -inline void FreeHeaders(MaybeStackBuffer* 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* trailers) { + const SubmitTrailers& submit_trailers) { DEBUG_HTTP2("Http2Session: prompting for trailers on stream %d\n", stream->id()); Local context = env()->context(); @@ -879,8 +838,8 @@ void Http2Session::OnTrailers(Nghttp2Stream* stream, if (ret->IsArray()) { Local headers = ret.As(); 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 index 49f6e55ebf377e..bf7134b2358435 100755 --- 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* 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 index 78379c6deabd3c..eae76430f46725 100755 --- 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 index c71c2f286efdc5..3ec6529f98145d 100644 --- 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 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 index bf2300be97b4d8..b3ccabfbbc9c6c 100755 --- 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* 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);