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.

Sign up for GitHub

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

Jump to bottom

[v12.x backport] http: reset headers timeout on headers complete #35819

Closed
wants to merge 1 commit into from
Closed

[v12.x backport] http: reset headers timeout on headers complete #35819

wants to merge 1 commit into from

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Oct 26, 2020 •
edited
Loading

headers timeout should not occur after headers have been
received.

Fixes: #35661

PR-URL: #34578
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Fedor Indutny [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: James M Snell [email protected]
Reviewed-By: Pranshu Srivastava [email protected]
(cherry picked from commit da4d8de)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v12.x labels Oct 26, 2020
@orgads orgads changed the title http: reset headers timeout on headers complete [v12.x backport] http: reset headers timeout on headers complete Oct 26, 2020
@orgads
Copy link
Contributor Author

orgads commented Oct 26, 2020

Looks like another commit is missing?

@orgads orgads mentioned this pull request Oct 26, 2020
Closed
@orgads
Copy link
Contributor Author

orgads commented Oct 26, 2020

The test passed 1000 as 1e3, which is double, not int32. Probably another commit is missing.

@orgads orgads force-pushed the http-timeout branch 2 times, most recently from 3225d5d to 9828efd Compare October 26, 2020 19:17
@Flarna
Copy link
Member

Flarna commented Oct 26, 2020

The test passed 1000 as 1e3, which is double, not int32. Probably another commit is missing.

Javascript has only number....

Maybe include #34609 in the backport PR as it fixes something in the test added here.

@ronag
Copy link
Member

ronag commented Oct 26, 2020

@orgads have you confirmed this actually fixes #35661?

@orgads
Copy link
Contributor Author

orgads commented Oct 26, 2020

The tests pass now (The arguments to parser.initialize are different in 14. I adapted them), and I confirmed it fixes the issue.

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 26, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Flarna
Copy link
Member

Flarna commented Oct 26, 2020

The test fails look like that ones fixed by #34609

@orgads
Copy link
Contributor Author

orgads commented Oct 26, 2020

It no longer fails. Unfortunately, the CI keeps running and notifying even after force-push...

@orgads
Copy link
Contributor Author

orgads commented Oct 26, 2020

Oops, didn't notice the bsd tests. Yes, it looks like it. I'll amend.

@ronag @orgads
http: reset headers timeout on headers complete
0573a69 
headers timeout should not occur *after* headers have been
received.

Fixes: nodejs#35661

PR-URL: nodejs#34578
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Pranshu Srivastava <[email protected]>
(cherry picked from commit da4d8de)
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@watilde
Copy link
Member

watilde commented Oct 27, 2020

I tested in my local with https://github.com/neversun/node-issue-35661 and this branch works.
@orgads Could you update the target branch to 12.x-staging?

@watilde
Copy link
Member

watilde commented Oct 27, 2020 •
edited
Loading

Let me ask for 👍 to fast track as this PR fixes a regression reported at #35661.

cc @nodejs/releasers

@richardlau
Copy link
Member

Let me ask for 👍 to fast track as this PR fixes a regression reported at #35661.

cc @nodejs/releasers

This needs to be targeted at v12.x-staging and CI rerun (as there are commits there that are not in v12.x.

@watilde watilde changed the base branch from v12.x to v12.x-staging October 27, 2020 04:52
@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020
@watilde
Copy link
Member

watilde commented Oct 27, 2020 •
edited
Loading

Agreed as I commented the same earlier and I should have arranged that before asking for fast-track. Then I just updated the targeted branch to v12.x-staging and triggered CI again. Once CI gets green light, I'll put fast-track label.

This needs to be targeted at v12.x-staging and CI rerun (as there are commits there that are not in v12.x.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@watilde watilde added fast-track PRs that do not need to wait for 48 hours to land. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Oct 27, 2020
@watilde
Copy link
Member

watilde commented Oct 30, 2020

I took the fast-track since 48h or more has passed, but I still think this PR remains a high priority as it fixes the regression.

cc @nodejs/releasers

@orgads
Copy link
Contributor Author

orgads commented Nov 2, 2020

What are we waiting for?

@watilde
Copy link
Member

watilde commented Nov 2, 2020

Let me cc @nodejs/backporters who can land this commit onto the staging branch.

Refs: https://github.com/nodejs/node/blob/7794d36a372b7cb448f5a637765a8fe2e1431d95/doc/guides/collaborator-guide.md#how-are-lts-branches-managed

@MylesBorins
Copy link
Contributor

@watilde we'll review this and all 12.x backports before cutting the RC

fwiw fast track isn't necessary for backports as they don't have the same timing requirement as they have previously been reviewed

@codebytere
Copy link
Member

Landed in 9f3e888

@codebytere codebytere closed this Nov 2, 2020
codebytere pushed a commit that referenced this pull request Nov 2, 2020
@ronag @codebytere
http: reset headers timeout on headers complete
9f3e888 
headers timeout should not occur *after* headers have been
received.

Fixes: #35661

PR-URL: #34578
Backport-PR-URL: #35819
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Pranshu Srivastava <[email protected]>
(cherry picked from commit da4d8de)
@orgads orgads deleted the http-timeout branch November 3, 2020 00:34
@watilde
Copy link
Member

watilde commented Nov 3, 2020

@MylesBorins Thank you for letting me know it!

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@ronag @MylesBorins
http: reset headers timeout on headers complete
f10d721 
headers timeout should not occur *after* headers have been
received.

Fixes: #35661

PR-URL: #34578
Backport-PR-URL: #35819
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Pranshu Srivastava <[email protected]>
(cherry picked from commit da4d8de)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickyes rickyes rickyes approved these changes

@Flarna Flarna Flarna approved these changes

@ronag ronag ronag approved these changes

@watilde watilde watilde approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@orgads @nodejs-github-bot @Flarna @ronag @watilde @richardlau @MylesBorins @codebytere @fhinkel @rickyes