Limit requests per connection #40082

fatal10110 · 2021-09-11T12:20:07Z

Trying to close #40071

Still missing tests and docs

fatal10110 · 2021-09-11T12:34:15Z

lib/_http_server.js

@@ -486,6 +487,7 @@ function connectionListenerInternal(server, socket) {
    // need to pause TCP socket/HTTP parser, and wait until the data will be
    // sent to the client.
    outgoingData: 0,
+    requestsCount: 0,


Not sure yet if state is per all sockets or there is a state per socket

this is for each individual socket

lib/_http_outgoing.js

mcollina

If you start adding unit tests (even failing) it would help.

mcollina · 2021-09-11T19:41:26Z

lib/_http_server.js

@@ -876,6 +878,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {

  const res = new server[kServerResponse](req);
  res._keepAliveTimeout = server.keepAliveTimeout;
+  res._maxRequestsPerSocket = server.maxRequestsPerSocket;


Why is this copied here?

I'm passing maxRequestsPerSocket to res since I need to set the max value on "keep-alive` header on response
https://github.com/nodejs/node/pull/40082/files/64132000c63934af235ae59095faad2015b3a045#diff-48d21edbddb6e855d1ee5716c49bcdc0d913c11ee8a24a98ea7dbc60cd253556R463
Didn't found a way to do it better

mcollina · 2021-09-11T19:43:53Z

lib/_http_server.js

+
+    if (server.maxRequestsPerSocket < ++state.requestsCount) {
+      res.writeHead(503);
+      res.end();


Is connection close correctly set here? I think shouldKeepAlive should be set to false.

mcollina · 2021-09-11T19:45:38Z

lib/_http_server.js

+      res.end();
+    }
+
+    res.shouldKeepAlive = server.maxRequestsPerSocket > state.requestsCount


Suggested change

res.shouldKeepAlive = server.maxRequestsPerSocket > state.requestsCount

res.shouldKeepAlive = server.maxRequestsPerSocket >= state.requestsCount

I think this should go to false if they are the same.

In the current code the shouldKeepAlive will be false if they are equal
the shouldKeepAlive is set to true only if requestsCount is small enough

server.maxRequestsPerSocket > state.requestsCount

fatal10110 · 2021-09-11T20:34:10Z

If you start adding unit tests (even failing) it would help.

Sure, Im on it right now, it just a bit complicated for me to understand how to run them (only my tests)

mcollina · 2021-09-11T20:36:55Z

./node test/parallel/test-your-new-test.js

mcollina

can you please add another test with a pipelined request coming in before the "last" (allowed) one completes.

fatal10110 · 2021-09-12T19:27:28Z

can you please add another test with a pipelined request coming in before the "last" (allowed) one completes.

I created a test sending 4 requests in a row with a timeout on the server side before response (timeout 5 sec) it is for sure received after the the last allow is completed, also see the code is reaching the 503 handler

        res.writeHead(503);
        res.end();

But it is not returned from the server (503) as a response, I see only the first 3
My guess is because the last one (the third one is "marked" as last=true because of shouldKeepAlive=false
not sure what to do now.

This is what I get as a response for 4 requests when limit is set to 3

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:17:24 GMT
Connection: keep-alive
Keep-Alive: timeout=5, max=3
Transfer-Encoding: chunked

c
Hello World!
0

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:17:24 GMT
Connection: keep-alive
Keep-Alive: timeout=5, max=3
Transfer-Encoding: chunked

c
Hello World!
0

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:17:24 GMT
Connection: close
Transfer-Encoding: chunked

c
Hello World!
0

fatal10110 · 2021-09-12T19:33:41Z

I used another flag instead of shouldKeepAlive and it worked, but there may be side effects if shouldKeepAlive is actually true,
but I send connection: close

results:

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:32:02 GMT
Connection: keep-alive
Keep-Alive: timeout=5, max=3
Transfer-Encoding: chunked

c
Hello World!
0

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:32:02 GMT
Connection: keep-alive
Keep-Alive: timeout=5, max=3
Transfer-Encoding: chunked

c
Hello World!
0

HTTP/1.1 200 OK
Content-Type: text/plain
Date: Sun, 12 Sep 2021 19:32:02 GMT
Connection: close
Transfer-Encoding: chunked

c
Hello World!
0

HTTP/1.1 503 Service Unavailable
Date: Sun, 12 Sep 2021 19:31:52 GMT
Connection: close
Transfer-Encoding: chunked

0

lib/_http_outgoing.js

nodejs-github-bot · 2021-09-13T13:24:24Z

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

mcollina · 2021-09-13T13:26:50Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1051/

fatal10110 · 2021-09-13T17:28:00Z

not sure what is "test-asan" and why it failed on timeout

mcollina

According to

https://datatracker.ietf.org/doc/html/draft-thomson-hybi-http-timeout-03#section-2.2.1

The "max" parameter has been used to indicate the maximum number of
requests that would be made on the connection. This parameter is
deprecated. Any limit on requests can be enforced by sending
"Connection: close" and closing the connection.

max is deprecated and should not be sent.

Adding a limit is actually fine and could help in production deployments let's just add that.

fatal10110 · 2021-09-14T11:17:54Z

Done, thanks!

artur-ma · 2021-09-15T23:20:05Z

@mcollina anything else should be done? or should I close this PR ?

nodejs-github-bot · 2021-09-19T06:34:13Z

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

artur-ma · 2021-09-19T09:45:21Z

Filed again with some strange error

10:08:27   duration_ms: 21.359
10:08:27   severity: fail
10:08:27   exitcode: 1
10:08:27   stack: |-
10:08:27     /home/iojs/build/workspace/node-test-binary-arm/test/common/debugger.js:84
10:08:27               reject(new Error([
10:08:27                      ^
10:08:27     
10:08:27     Error: Timeout (10000) while waiting for /(?:assert|break|break on start|debugCommand|exception|other|promiseRejection) in/i; found: < Debugger ending on ws://127.0.0.1:9229/0161d4a8-8f01-4590-b7e9-18412e6e9a13
10:08:27     < For help, see: https://nodejs.org/en/docs/inspector
10:08:27     < 
10:08:27     debug> 
10:08:27     
10:08:27     < Debugger listening on ws://127.0.0.1:9229/f8ab1b5e-afa3-4a18-8975-1d76592e7ade
10:08:27     < For help, see: https://nodejs.org/en/docs/inspector
10:08:27     < 
10:08:27     
10:08:27     debug> 
10:08:27     
10:08:27     connecting to 127.0.0.1:9229 ...
10:08:27      ok
10:08:27     
10:08:27     debug> 
10:08:27     
10:08:27     < Debugger attached.
10:08:27     < 
10:08:27     debug> 
10:08:27         at Timeout.<anonymous> (/home/iojs/build/workspace/node-test-binary-arm/test/common/debugger.js:84:18)
10:08:27         at listOnTimeout (node:internal/timers:557:17)
10:08:27         at processTimers (node:internal/timers:500:7)
10:08:27     
10:08:27     Node.js v17.0.0-pre
10:08:27   ...

nodejs-github-bot · 2021-09-19T10:33:37Z

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

artur-ma · 2021-09-19T12:00:50Z

Should be any semver label here? (minor/major)

mcollina

lgtm

mcollina · 2021-09-19T14:29:12Z

Landed in 4e8f11d

Fixes: #40071 PR-URL: #40082 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO

Fixes: #40071 PR-URL: #40082 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: #40175

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 11, 2021

fatal10110 force-pushed the max_request_per_socket branch from 2b5d67f to 661251d Compare September 11, 2021 12:23

fatal10110 commented Sep 11, 2021

View reviewed changes

mcollina reviewed Sep 11, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

mcollina reviewed Sep 11, 2021

View reviewed changes

fatal10110 closed this Sep 11, 2021

fatal10110 reopened this Sep 11, 2021

fatal10110 force-pushed the max_request_per_socket branch from 12d6866 to d64d618 Compare September 12, 2021 13:55

mcollina reviewed Sep 12, 2021

View reviewed changes

fatal10110 commented Sep 12, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

fatal10110 force-pushed the max_request_per_socket branch from f533750 to 84d4e8e Compare September 12, 2021 19:47

fatal10110 marked this pull request as ready for review September 12, 2021 19:48

fatal10110 requested a review from mcollina September 12, 2021 19:48

ronag mentioned this pull request Sep 13, 2021

Implement keep-alive max request hint support nodejs/undici#1030

Closed

github-actions bot mentioned this pull request Sep 14, 2021

CI Reliability 2021-09-14 nodejs/reliability#58

Open

19 tasks

mcollina requested changes Sep 14, 2021

View reviewed changes

fatal10110 force-pushed the max_request_per_socket branch from 2ed33eb to 4d0ca75 Compare September 14, 2021 11:16

fatal10110 requested a review from mcollina September 14, 2021 11:17

fatal10110 force-pushed the max_request_per_socket branch from 4d0ca75 to 5b35ffe Compare September 14, 2021 11:18

github-actions bot mentioned this pull request Sep 15, 2021

CI Reliability 2021-09-15 nodejs/reliability#59

Open

20 tasks

fatal10110 force-pushed the max_request_per_socket branch from 5b35ffe to c5ae814 Compare September 16, 2021 21:32

This was referenced Sep 17, 2021

CI Reliability 2021-09-17 nodejs/reliability#61

Open

CI Reliability 2021-09-18 nodejs/reliability#62

Open

CI Reliability 2021-09-19 nodejs/reliability#63

Open

mcollina added semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Sep 19, 2021

mcollina approved these changes Sep 19, 2021

View reviewed changes

mcollina pushed a commit that referenced this pull request Sep 19, 2021

http: limit requests per connection

4e8f11d

Fixes: #40071 PR-URL: #40082 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

mcollina closed this Sep 19, 2021

github-actions bot mentioned this pull request Sep 20, 2021

CI Reliability 2021-09-20 nodejs/reliability#64

Open

17 tasks

BethGriggs pushed a commit that referenced this pull request Sep 21, 2021

http: limit requests per connection

3851065

Fixes: #40071 PR-URL: #40082 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

BethGriggs pushed a commit that referenced this pull request Sep 21, 2021

http: limit requests per connection

a63a4bc

Fixes: #40071 PR-URL: #40082 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

BethGriggs mentioned this pull request Sep 21, 2021

v16.10.0 proposal #40175

Merged

1 task

github-actions bot mentioned this pull request Sep 22, 2021

CI Reliability 2021-09-22 nodejs/reliability#65

Open

15 tasks

github-actions bot mentioned this pull request Sep 23, 2021

CI Reliability 2021-09-23 nodejs/reliability#66

Open

18 tasks

mcollina mentioned this pull request Sep 23, 2021

Lack of consistency in maxRequestPerSocket #40190

Closed

This was referenced Sep 24, 2021

CI Reliability 2021-09-24 nodejs/reliability#67

Open

CI Reliability 2021-09-25 nodejs/reliability#68

Open

CI Reliability 2021-09-26 nodejs/reliability#69

Open

CI Reliability 2021-09-27 nodejs/reliability#70

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit requests per connection #40082

Limit requests per connection #40082

fatal10110 commented Sep 11, 2021 •

edited

Loading

fatal10110 Sep 11, 2021

mcollina Sep 14, 2021

mcollina left a comment

mcollina Sep 11, 2021

fatal10110 Sep 11, 2021 •

edited

Loading

mcollina Sep 11, 2021

mcollina Sep 11, 2021

fatal10110 Sep 11, 2021

fatal10110 commented Sep 11, 2021

mcollina commented Sep 11, 2021 •

edited

Loading

mcollina left a comment

fatal10110 commented Sep 12, 2021 •

edited

Loading

fatal10110 commented Sep 12, 2021 •

edited

Loading

nodejs-github-bot commented Sep 13, 2021

mcollina commented Sep 13, 2021

fatal10110 commented Sep 13, 2021

mcollina left a comment

fatal10110 commented Sep 14, 2021

artur-ma commented Sep 15, 2021

nodejs-github-bot commented Sep 19, 2021

artur-ma commented Sep 19, 2021

nodejs-github-bot commented Sep 19, 2021

artur-ma commented Sep 19, 2021

mcollina left a comment

mcollina commented Sep 19, 2021

	res.shouldKeepAlive = server.maxRequestsPerSocket > state.requestsCount
	res.shouldKeepAlive = server.maxRequestsPerSocket >= state.requestsCount

Limit requests per connection #40082

Limit requests per connection #40082

Conversation

fatal10110 commented Sep 11, 2021 • edited Loading

fatal10110 Sep 11, 2021

Choose a reason for hiding this comment

mcollina Sep 14, 2021

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina Sep 11, 2021

Choose a reason for hiding this comment

fatal10110 Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

mcollina Sep 11, 2021

Choose a reason for hiding this comment

mcollina Sep 11, 2021

Choose a reason for hiding this comment

fatal10110 Sep 11, 2021

Choose a reason for hiding this comment

fatal10110 commented Sep 11, 2021

mcollina commented Sep 11, 2021 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

fatal10110 commented Sep 12, 2021 • edited Loading

fatal10110 commented Sep 12, 2021 • edited Loading

nodejs-github-bot commented Sep 13, 2021

mcollina commented Sep 13, 2021

fatal10110 commented Sep 13, 2021

mcollina left a comment

Choose a reason for hiding this comment

fatal10110 commented Sep 14, 2021

artur-ma commented Sep 15, 2021

nodejs-github-bot commented Sep 19, 2021

artur-ma commented Sep 19, 2021

nodejs-github-bot commented Sep 19, 2021

artur-ma commented Sep 19, 2021

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 19, 2021

fatal10110 commented Sep 11, 2021 •

edited

Loading

fatal10110 Sep 11, 2021 •

edited

Loading

mcollina commented Sep 11, 2021 •

edited

Loading

fatal10110 commented Sep 12, 2021 •

edited

Loading

fatal10110 commented Sep 12, 2021 •

edited

Loading