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.

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

Limit requests per connection #40082

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
http: limit requests per connection
  • Loading branch information
fatal10110 committed Sep 16, 2021
commit 0ab2852470416954d5ce6c82617f1657ea0a3c60
1 change: 1 addition & 0 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ function OutgoingMessage() {
this._header = null;
this[kOutHeaders] = null;

this._maxRequestsPerSocket = null;
this._keepAliveTimeout = 0;

this._onPendingData = nop;
Expand Down
13 changes: 13 additions & 0 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ function Server(options, requestListener) {
this.timeout = 0;
this.keepAliveTimeout = 5000;
this.maxHeadersCount = null;
this.maxRequestsPerSocket = null;
this.headersTimeout = 60 * 1000; // 60 seconds
this.requestTimeout = 0;
}
Expand Down Expand Up @@ -485,6 +486,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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for each individual socket

keepAliveTimeoutSet: false
};
state.onData = socketOnData.bind(undefined,
Expand Down Expand Up @@ -875,6 +877,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this copied here?

Copy link
Contributor Author

@fatal10110 fatal10110 Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

res._onPendingData = updateOutgoingData.bind(undefined,
socket, state);

Expand Down Expand Up @@ -903,6 +906,16 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
resOnFinish.bind(undefined,
req, res, socket, state, server));

if (req.httpVersionMajor === 1 && req.httpVersionMinor === 1
&& typeof server.maxRequestsPerSocket === 'number'
&& server.maxRequestsPerSocket > ++state.requestsCount) {
res.shouldKeepAlive = false;
res.writeHead(503, {
'Connection': 'close'
});
res.end();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (req.headers.expect !== undefined &&
(req.httpVersionMajor === 1 && req.httpVersionMinor === 1)) {
if (RegExpPrototypeTest(continueExpression, req.headers.expect)) {
Expand Down