★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/32069Nouvelle recherche | Portail wanayoo
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 jasnell@gmail.com

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 <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ lib / src labels Mar 3, 2020
@jasnell jasnell requested a review from addaleax Mar 3, 2020
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 Mar 4, 2020
Co-Authored-By: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 4, 2020

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 4, 2020

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 4, 2020

@jasnell jasnell added the author ready label Mar 4, 2020
@jasnell jasnell requested review from danbev, lundibundi and mcollina Mar 5, 2020
Copy link
Member

@mcollina mcollina left a comment

lgtm

jasnell added a commit that referenced this issue 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 <jasnell@gmail.com>

PR-URL: #32069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

@jasnell 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 issue 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 <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
cjihrig added a commit to cjihrig/node that referenced this issue 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 <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins added a commit that referenced this issue 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 <jasnell@gmail.com>

PR-URL: #32069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue 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 <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins added a commit that referenced this issue 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 <shishugi@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
@codebytere
Copy link
Member

@codebytere codebytere commented Mar 16, 2020

@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!

@codebytere codebytere added the backport-requested-v12.x label Mar 16, 2020
@targos targos removed the author ready label Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v12.x c++ lib / src
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants