Skip to content

http: null the joinDuplicateHeaders property on cleanup - #48608

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
lpinca:clean/http-parser
Jul 3, 2023
Merged

http: null the joinDuplicateHeaders property on cleanup#48608
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
lpinca:clean/http-parser

Conversation

@lpinca

@lpincalpinca commented Jun 30, 2023

Copy link
Copy Markdown
Member

Null the joinDuplicateHeaders property when the parser is freed.

Refs: #45982

@lpinca
http: null the joinDuplicateHeaders property on cleanup
03ad516
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: nodejs#45982
@nodejs-github-bot

nodejs-github-bot commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 30, 2023
marco-ippolito
marco-ippolito approved these changes Jun 30, 2023
View reviewed changes

@marco-ippolitomarco-ippolito left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@lpinca

lpinca commented Jun 30, 2023

Copy link
Copy Markdown
MemberAuthor

This deflakes the parallel/test-http-request-join-authorization-headers test. The problem was that a parser with the property set to true was picked up on the server. See https://ci.nodejs.org/job/node-test-pull-request/52475/

not ok 1251 parallel/test-http-request-join-authorization-headers
---
duration_ms: 245.54800
severity: fail
exitcode: 1
stack: |-
node:assert:125
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
'1, 2' !== '1'
at Server.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx11-x64/test/parallel/test-http-request-join-authorization-headers.js:39:12)
at Server.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx11-x64/test/common/index.js:466:15)
at Server.emit (node:events:512:28)
at parserOnIncoming (node:_http_server:1125:12)
at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: '1, 2',
expected: '1',
operator: 'strictEqual'
}
Node.js v21.0.0-pre

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
lpinca
lpinca commented Jun 30, 2023
View reviewed changes
Comment threadtest/parallel/test-http-parser-memory-retention.js
server.listen(common.mustCall(() => {
const request = http.get({ port: server.address().port });
const request = http.get({
headers: { Connection: 'close' },

@lpincalpincaJun 30, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is added only to speed up the test. There is no need to wait for the keep-alive timeout to expire.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52567/

mcollina
mcollina approved these changes Jun 30, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

nodejs-github-bot commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52568/

@github-actionsgithub-actionsBot mentioned this pull request Jul 1, 2023
Open
30 tasks
This was referenced Jul 2, 2023
Open
Open
@lpincalpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 651c02c into nodejs:mainJul 3, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 3, 2023

Copy link
Copy Markdown
Collaborator

Landed in 651c02c

@lpinca
lpinca deleted the clean/http-parser branch July 3, 2023 06:22
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@lpinca@RafaelGSS
http: null the joinDuplicateHeaders property on cleanup
7d8a0b6
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jul 3, 2023
Merged
This was referenced Jul 4, 2023
Open
Open
Open
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca@Ceres6
http: null the joinDuplicateHeaders property on cleanup
baf8d33
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: nodejs#45982
PR-URL: nodejs#48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca@Ceres6
http: null the joinDuplicateHeaders property on cleanup
ba1cf4b
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: nodejs#45982
PR-URL: nodejs#48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@lpinca@ruyadorno
http: null the joinDuplicateHeaders property on cleanup
27d6833
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@ruyadornoruyadorno mentioned this pull request Sep 11, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@lpinca@ruyadorno
http: null the joinDuplicateHeaders property on cleanup
eb4550a
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@lpinca@ruyadorno
http: null the joinDuplicateHeaders property on cleanup
01746c7
Null the `joinDuplicateHeaders` property when the parser is freed.
Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@marco-ippolitomarco-ippolitomarco-ippolito approved these changes
@KhafraDevKhafraDevKhafraDev approved these changes

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lpinca@nodejs-github-bot@mcollina@benjamingr@marco-ippolito@KhafraDev