Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,http2: DRY header/trailer handling code up #14688

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
src,http2: DRY header/trailer handling code up
Remove duplicate code through minor refactoring.
  • Loading branch information
addaleax committed Aug 8, 2017
commit d33f3eb6b7e1f45ad5b9848f42144d957fa35c02
47 changes: 3 additions & 44 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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(*trailers, trailers.length());
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions src/node_http2_core-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
31 changes: 15 additions & 16 deletions src/node_http2_core.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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::operator ()(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.
Expand Down
21 changes: 19 additions & 2 deletions src/node_http2_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 operator ()(nghttp2_nv* trailers, size_t length) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this a normal method called .Send() or whatever is clearer, IMO. No risk of confusing method calls with plain function calls.


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);
Expand Down