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: introduce node_http_common #32069

Closed
wants to merge 3 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 3, 2020

The nghttp2 and nghttp3 (used in the QUIC implementation) share nearly
identical structs for header handling. However, they differ enough that
they need to be handled slightly different in each case. This PR
includes some elements introduced in the QUIC PR separated out to
make them independently reviewable, and updates the http2 implementation
to use the shared utilities.

@addaleax @danbev ... just fyi... this includes a number of additional changes that are not in the nodejs/quic repo version. Specifically, the http/2 bits, but also the implementation of the utilities has evolved somewhat so it can definitely use another look.

Signed-off-by: James M Snell [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

The nghttp2 and nghttp3 (used in the QUIC implementation) share nearly
identical structs for header handling. However, they differ enough that
they need to be handled slightly different in each case. This PR
includes some elements introduced in the QUIC PR separated out to
make them independently reviewable, and updates the http2 implementation
to use the shared utilities.

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 3, 2020
@jasnell jasnell requested a review from addaleax March 3, 2020 18:23
src/node_http2.cc Outdated Show resolved Hide resolved
src/node_http_common-inl.h Outdated Show resolved Hide resolved
src/node_http_common.h Outdated Show resolved Hide resolved
src/node_http_common.h Outdated Show resolved Hide resolved
jasnell and others added 2 commits March 4, 2020 08:21
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

jasnell added a commit that referenced this pull request Mar 6, 2020
The nghttp2 and nghttp3 (used in the QUIC implementation) share nearly
identical structs for header handling. However, they differ enough that
they need to be handled slightly different in each case. This PR
includes some elements introduced in the QUIC PR separated out to
make them independently reviewable, and updates the http2 implementation
to use the shared utilities.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32069
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Mar 6, 2020

Landed in 434d39d

@jasnell jasnell closed this Mar 6, 2020
@cjihrig cjihrig mentioned this pull request Mar 6, 2020
2 tasks
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 8, 2020
../src/node_http_common.h:497:8: warning: 'MemoryInfo' overrides a
member function but is not marked 'override' [-Winconsistent-missing-override]
  void MemoryInfo(MemoryTracker* tracker) const {

PR-URL: nodejs#32126
Refs: nodejs#32069
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 8, 2020
../src/node_http_common-inl.h:126:7: warning: field 'token_'
will be initialized after field 'name_' [-Wreorder]
    : token_(other.token_),

PR-URL: nodejs#32126
Refs: nodejs#32069
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
The nghttp2 and nghttp3 (used in the QUIC implementation) share nearly
identical structs for header handling. However, they differ enough that
they need to be handled slightly different in each case. This PR
includes some elements introduced in the QUIC PR separated out to
make them independently reviewable, and updates the http2 implementation
to use the shared utilities.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32069
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
../src/node_http_common.h:497:8: warning: 'MemoryInfo' overrides a
member function but is not marked 'override' [-Winconsistent-missing-override]
  void MemoryInfo(MemoryTracker* tracker) const {

PR-URL: #32126
Refs: #32069
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
../src/node_http_common-inl.h:126:7: warning: field 'token_'
will be initialized after field 'name_' [-Wreorder]
    : token_(other.token_),

PR-URL: #32126
Refs: #32069
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
@codebytere
Copy link
Member

@jasnell could you backport this to v12.x if you think it's applicable? I'll set the label but feel free to update it!

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants