http: fix for handling on boot timers headers and request #48291

franciszek-koltuniuk-red · 2023-06-02T06:25:51Z

This change is a fix for handling headersTimeout and requestTimeout
that causes unexpected behavior if the HTTP server is started on boot:

the connections to the server can be closed immediately
with the status HTTP 408

This issue usually happens in IoT or embedded devices where
the reference timestamp (returned by uv_hrtime()) is counted since boot
and can be smaller than the headersTimeout or the requestTimeout value.

Additionally added performance improvement to process the list of
connection only if one of the timers should be processed

nodejs-github-bot · 2023-06-02T06:25:56Z

Review requested:

@nodejs/http
@nodejs/net

ShogunPanda · 2023-06-02T06:48:41Z

In general LGTM. Can you please add a test?

franciszek-koltuniuk-red · 2023-06-02T07:41:22Z

I try to prepare a test case for this, but the issue is observed when the now is less than headersTimeout or requestTimeout where now is :
const uint64_t now = uv_hrtime();
I don't know how to create test like test/parallel/test-http-server-request-timeout-interrupted-body.js to fulfill this requirement in all environments
The value for headersTimeout or requestTimeout is limited to Integer see validation requestTimeout and headersTimeout

franciszek-koltuniuk-red · 2023-06-02T07:55:21Z

This issue is observed since "refactor headersTimeout and requestTimeout logic" #41263

ShogunPanda · 2023-06-02T09:33:13Z

I don't know how to create test like test/parallel/test-http-server-request-timeout-interrupted-body.js to fulfill this requirement in all environments

@nodejs/libuv Do you have any opinion on this? Is there a way to mock uv_hrtime?

bnoordhuis · 2023-06-02T16:52:05Z

Is there a way to mock uv_hrtime?

In libuv? No.¹ You can of course refactor node's code to make hrtime calls pluggable but that's overkill for a simple fix like this.

¹ Or rather, you can overload clock_gettime/mach_absolute_time/mach_continuous_time/etc. through LD_PRELOAD/DYLD_PRELOAD but that's probably not what you mean.

ShogunPanda · 2023-06-02T17:08:18Z

Yeah, definitely too much for a simple verification.
The next run on the connectionsCheckingInterval would kill connections anyway.

Are we good in landing this without a test?

benjamingr

If we can't test this - it would be good to at least add a comment to why this change is being made in the code.

jasnell

LGTM but I agree that additional comments would be helpful.

franciszek-koltuniuk-red · 2023-06-05T06:57:13Z

additional comments would be helpful.

Code commented

This change is a fix for handling headersTimeout and requestTimeout that causes unexpected behavior if the HTTP server is started on boot: - the connections to the server can be closed immediately with the status HTTP 408 This issue usually happens on IoT or embedded devices where the reference timestamp (returned by uv_hrtime()) is counted since boot and can be smaller than the headersTimeout or the requestTimeout value. Additionally added performance improvement to process the list of connection only if one of the timers should be processed

franciszek-koltuniuk-red · 2023-06-07T15:03:55Z

I had to re-edit commits messages, to fulfill the commit message check

nodejs-github-bot · 2023-06-08T07:55:15Z

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

mcollina

lgtm

franciszek-koltuniuk-red · 2023-06-13T11:15:49Z

I see that checks failed but these fails are not related to this change:

node-test-commit — tests failed due to dependency on node-test-commit-osx, node-test-commit-windows-fanned, node-test-binary-windows-js-suites
node-test-commit-osx — tests failed due to:
10:45:24 Failed to write /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/out/Release/obj/gen/node_snapshot.cc 10:45:24 make[2]: *** [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/out/Release/obj/gen/node_snapshot.cc] Error 1
see: Failed to write /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/out/Release/obj/gen/node_snapshot.cc
node-test-commit-windows-fanned — tests failed due to dependency on node-test-binary-windows-js-suites
node-test-pull-reques — tests failed due to dependency on node-test-commit
node-test-binary-windows-js-suites — tests failed due to:
10:28:19 Can't clean tmpdir: C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.168 10:28:19 Files blocking: [ 'node.heapsnapshot' ] 10:28:19 10:28:19 C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:68 10:28:19 throw e; 10:28:19 ^ 10:28:19 10:28:19 Error: EBUSY: resource busy or locked, rmdir '\\?\C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.168'
see: Can't clean tmpdir: C:\workspace\node-test-binary-windows-js-suites\node\test.tmp.168

to summarize the test failed due to errors unrelated to this change.

What can I do to process this pull request?

btw.
I think that it would be desirable to backport this change to stable branches

nodejs-github-bot · 2023-06-14T08:24:13Z

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

nodejs-github-bot · 2023-06-14T13:27:34Z

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

franciszek-koltuniuk-red · 2023-06-19T07:54:27Z

What is blocking this change from being merged?

ShogunPanda · 2023-06-19T09:27:13Z

~~I tried to add the labels to merge this. But since CI is locked for upcoming security releases I'm not sure if this will affected as well.~~

I was completely wrong. Nevermind. Landed :)

nodejs-github-bot · 2023-06-19T09:33:04Z

Landed in 8e710c9

This change is a fix for handling headersTimeout and requestTimeout that causes unexpected behavior if the HTTP server is started on boot: - the connections to the server can be closed immediately with the status HTTP 408 This issue usually happens on IoT or embedded devices where the reference timestamp (returned by uv_hrtime()) is counted since boot and can be smaller than the headersTimeout or the requestTimeout value. Additionally added performance improvement to process the list of connection only if one of the timers should be processed PR-URL: #48291 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

franciszek-koltuniuk-red · 2023-07-13T12:01:31Z

@RafaelGSS who can decide to backport this fix to LTS branches?

This change is a fix for handling headersTimeout and requestTimeout that causes unexpected behavior if the HTTP server is started on boot: - the connections to the server can be closed immediately with the status HTTP 408 This issue usually happens on IoT or embedded devices where the reference timestamp (returned by uv_hrtime()) is counted since boot and can be smaller than the headersTimeout or the requestTimeout value. Additionally added performance improvement to process the list of connection only if one of the timers should be processed PR-URL: nodejs#48291 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This change is a fix for handling headersTimeout and requestTimeout that causes unexpected behavior if the HTTP server is started on boot: - the connections to the server can be closed immediately with the status HTTP 408 This issue usually happens on IoT or embedded devices where the reference timestamp (returned by uv_hrtime()) is counted since boot and can be smaller than the headersTimeout or the requestTimeout value. Additionally added performance improvement to process the list of connection only if one of the timers should be processed PR-URL: #48291 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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. needs-ci PRs that need a full CI run. labels Jun 2, 2023

ronag approved these changes Jun 2, 2023
View reviewed changes

benjamingr approved these changes Jun 3, 2023
View reviewed changes

jasnell approved these changes Jun 3, 2023
View reviewed changes

franciszek-koltuniuk-red changed the title ~~Headers request timers fix~~http: fix for handling on boot timers headers and request Jun 4, 2023

ShogunPanda approved these changes Jun 5, 2023
View reviewed changes

marco-ippolito approved these changes Jun 5, 2023
View reviewed changes

franciszek-koltuniuk-red force-pushed the headers-request-timers-fix branch from 590d23f to ae3f8caCompare June 7, 2023 14:24

franciszek-koltuniuk-red added 3 commits June 7, 2023 16:28

update after autochecks
cd272a6

update in sourcode: commented the change
fc3c854

franciszek-koltuniuk-red force-pushed the headers-request-timers-fix branch from ae3f8ca to fc3c854Compare June 7, 2023 14:30

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023

mcollina approved these changes Jun 8, 2023
View reviewed changes

This was referenced Jun 9, 2023
CI Reliability 2023-06-09 nodejs/reliability#588
Open
CI Reliability 2023-06-10 nodejs/reliability#589
Open

github-actionsbot mentioned this pull request Jun 15, 2023
CI Reliability 2023-06-15 nodejs/reliability#594
Open
22 tasks

tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 19, 2023

ShogunPanda added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2023

nodejs-github-bot merged commit 8e710c9 into nodejs:mainJun 19, 2023

github-actionsbot mentioned this pull request Jun 21, 2023
CI Reliability 2023-06-21 nodejs/reliability#595
Open
29 tasks

RafaelGSS mentioned this pull request Jul 3, 2023
v20.4.0 proposal #48643
Merged

das7pad mentioned this pull request Aug 31, 2023
requestTimeout and long running uploads #46574
Open

ruyadorno mentioned this pull request Sep 10, 2023
v18.18.0 proposal #49220
Merged

Uh oh!

http: fix for handling on boot timers headers and request #48291

http: fix for handling on boot timers headers and request #48291

Uh oh!

Conversation

franciszek-koltuniuk-red commented Jun 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 2, 2023

Uh oh!

ShogunPanda commented Jun 2, 2023

Uh oh!

franciszek-koltuniuk-red commented Jun 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franciszek-koltuniuk-red commented Jun 2, 2023

Uh oh!

ShogunPanda commented Jun 2, 2023

Uh oh!

bnoordhuis commented Jun 2, 2023

Uh oh!

ShogunPanda commented Jun 2, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

franciszek-koltuniuk-red commented Jun 5, 2023

Uh oh!

franciszek-koltuniuk-red commented Jun 7, 2023

Uh oh!

nodejs-github-bot commented Jun 8, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

franciszek-koltuniuk-red commented Jun 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 14, 2023

Uh oh!

nodejs-github-bot commented Jun 14, 2023

Uh oh!

franciszek-koltuniuk-red commented Jun 19, 2023

Uh oh!

ShogunPanda commented Jun 19, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 19, 2023

Uh oh!

franciszek-koltuniuk-red commented Jul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

franciszek-koltuniuk-red commented Jun 2, 2023•
edited
Loading

franciszek-koltuniuk-red commented Jun 2, 2023•
edited
Loading

franciszek-koltuniuk-red commented Jun 13, 2023•
edited
Loading

ShogunPanda commented Jun 19, 2023•
edited
Loading

franciszek-koltuniuk-red commented Jul 13, 2023•
edited
Loading