Skip to content

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-botnodejs-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
@ofirbarakofirbarakforce-pushed the http-prfixed-headers-order branch from cce4dce to 32f9aafCompareJanuary 28, 2022 21:36
Copy link
Member

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

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

@ofirbarakofirbarakforce-pushed the http-prfixed-headers-order branch from 378aa75 to cc03955CompareJanuary 30, 2022 13:10
@ofirbarakofirbarak changed the title lib: fix prefixed headers orderhttp2: fix prefixed headers orderJan 30, 2022
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@TrottTrott dismissed their stale reviewJanuary 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

Landed in ee4bd95

@jasnelljasnell 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]>
@ruyadornoruyadorno mentioned this pull request Feb 8, 2022
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.needs-ciPRs 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

@ofirbarak@nodejs-github-bot@jasnell@mcollina@Trott