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 <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 ae3f8ca Compare 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 fc3c854 Compare 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-actions bot 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-actions bot 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:main Jun 19, 2023

github-actions bot 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

franciszek-koltuniuk-red commented Jun 2, 2023 •

edited

Loading

nodejs-github-bot commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

franciszek-koltuniuk-red commented Jun 2, 2023 •

edited

Loading

franciszek-koltuniuk-red commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

bnoordhuis commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

benjamingr left a comment

jasnell left a comment

franciszek-koltuniuk-red commented Jun 5, 2023

franciszek-koltuniuk-red commented Jun 7, 2023

nodejs-github-bot commented Jun 8, 2023

mcollina left a comment

franciszek-koltuniuk-red commented Jun 13, 2023 •

edited

Loading

nodejs-github-bot commented Jun 14, 2023

nodejs-github-bot commented Jun 14, 2023

franciszek-koltuniuk-red commented Jun 19, 2023

ShogunPanda commented Jun 19, 2023 •

edited

Loading

nodejs-github-bot commented Jun 19, 2023

franciszek-koltuniuk-red commented Jul 13, 2023 •

edited

Loading

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

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

Conversation

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

nodejs-github-bot commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

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

franciszek-koltuniuk-red commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

bnoordhuis commented Jun 2, 2023

ShogunPanda commented Jun 2, 2023

benjamingr left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

franciszek-koltuniuk-red commented Jun 5, 2023

franciszek-koltuniuk-red commented Jun 7, 2023

nodejs-github-bot commented Jun 8, 2023

mcollina left a comment

Choose a reason for hiding this comment

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

nodejs-github-bot commented Jun 14, 2023

nodejs-github-bot commented Jun 14, 2023

franciszek-koltuniuk-red commented Jun 19, 2023

ShogunPanda commented Jun 19, 2023 • edited Loading

nodejs-github-bot commented Jun 19, 2023

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

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