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

http2: fix prefixed headers order #41735

Closed
wants to merge 4 commits into from

Conversation

ofirbarak
Copy link
Contributor

Keep prefixed headers order same as sent order

Fixes: #38797

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 28, 2022
@ofirbarak ofirbarak force-pushed the http-prfixed-headers-order branch from cce4dce to 32f9aaf Compare January 28, 2022 21:36
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.

Can you please add a unit test?

Trott
Trott previously requested changes Jan 29, 2022
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Welcome @ofirbarak and thanks for the pull request. Are you able to rebase to edit the first commit message so that it starts with http2: rather than lib:, and then force push to your branch?

If this change doesn't happen and some collaborator wants to land this anyway, that's fine, clear this objection, but please make sure to land manually so you can edit the commit message yourself. Thanks!

@ofirbarak ofirbarak force-pushed the http-prfixed-headers-order branch from 378aa75 to cc03955 Compare January 30, 2022 13:10
@ofirbarak ofirbarak changed the title lib: fix prefixed headers order http2: fix prefixed headers order Jan 30, 2022
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

@Trott Trott dismissed their stale review January 30, 2022 15:43

commit message updated

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 6, 2022

jasnell pushed a commit that referenced this pull request Feb 6, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 6, 2022

Landed in ee4bd95

@jasnell jasnell closed this Feb 6, 2022
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
Keep pseudo-headers order same as sent order

Fixes: nodejs#38797

PR-URL: nodejs#41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
Keep pseudo-headers order same as sent order

Fixes: nodejs#38797

PR-URL: nodejs#41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 8, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Http2 reverses order of headers starting with ":" prefix
5 participants