FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v12.x backport] http: reset headers timeout on headers complete by orgads · Pull Request #35819 · nodejs/node · GitHub

/ node Public

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

Closed
orgads wants to merge 1 commit into
nodejs:v12.x-stagingfrom
orgads:http-timeout
ConversationCommits (1)ChecksFiles changed
Closed

[v12.x backport] http: reset headers timeout on headers complete#35819
orgads wants to merge 1 commit into
nodejs:v12.x-stagingfrom
orgads:http-timeout

Conversation

orgads commented Oct 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

headers timeout should not occur after headers have been
received.

Fixes: #35661

PR-URL: #34578
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Fedor Indutny fedor.indutny@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Pranshu Srivastava rexagod@gmail.com
(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 commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

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 changed the title http: reset headers timeout on headers complete [v12.x backport] http: reset headers timeout on headers complete Oct 26, 2020

orgads commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

Looks like another commit is missing?

orgads mentioned this pull request Oct 26, 2020
Closed

orgads commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

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

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

Flarna commented Oct 26, 2020

Copy link
Copy Markdown
Member

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 commented Oct 26, 2020

Copy link
Copy Markdown
Member

@orgads have you confirmed this actually fixes #35661?

orgads commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

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

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 outstanding review comments, and a CI started. labels Oct 26, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020

This comment has been minimized.

Sign in to view

Flarna commented Oct 26, 2020

Copy link
Copy Markdown
Member

The test fails look like that ones fixed by #34609

orgads commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

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

orgads commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

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

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 <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
(cherry picked from commit da4d8de)
Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

watilde commented Oct 27, 2020

Copy link
Copy Markdown
Member

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 commented Oct 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

cc @nodejs/releasers

richardlau commented Oct 27, 2020

Copy link
Copy Markdown
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 changed the base branch from v12.x to v12.x-staging October 27, 2020 04:52
watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020

watilde commented Oct 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020

nodejs-github-bot commented Oct 27, 2020

Copy link
Copy Markdown
Collaborator

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

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 commented Oct 30, 2020

Copy link
Copy Markdown
Member

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 commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

What are we waiting for?

watilde commented Nov 2, 2020

Copy link
Copy Markdown
Member

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 commented Nov 2, 2020

Copy link
Copy Markdown
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 commented Nov 2, 2020

Copy link
Copy Markdown
Member

Landed in 9f3e888

codebytere closed this Nov 2, 2020
codebytere pushed a commit that referenced this pull request Nov 2, 2020
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 <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
(cherry picked from commit da4d8de)
orgads deleted the http-timeout branch November 3, 2020 00:34

watilde commented Nov 3, 2020

Copy link
Copy Markdown
Member

@MylesBorins Thank you for letting me know it!

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
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 <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>
(cherry picked from commit da4d8de)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

watilde watilde approved these changes
ronag ronag approved these changes
Flarna Flarna approved these changes
+2 more reviewers
fhinkel fhinkel approved these changes
rickyes rickyes approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL