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.

Sign up for GitHub

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

Jump to bottom

http: wait for pending responses in closeIdleConnections #43890

Closed
wants to merge 1 commit into from
Closed

http: wait for pending responses in closeIdleConnections #43890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,10 @@ Server.prototype.closeIdleConnections = function() {
const connections = this[kConnections].idle();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about the definition of this[kConnections].idle(). If it has an incoming message in the socket, why it will also defined as idleConnections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, it's a bit confusing.
The definition of "idle", "active", "expired" and so forth must be interpreted from the point of view of the server inbound parser.

In this case a socket is considered idle because it has finished sending the request which means that (ignoring HTTP pipelining)no new data is expected to be received. The socket is therefore either waiting for a response or in HTTP Keep-Alive mode.

Is that clear now?


for (let i = 0, l = connections.length; i < l; i++) {
if (connections[i].socket._httpMessage && !connections[i].socket._httpMessage.finished) {
Copy link
Member

Choose a reason for hiding this comment

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

I am worried this addition will make #43795 worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we gotta fix that test (next on my list for Monday), but the server current does not behaves as the documentations says (and the docs reflects the original intended behavior) so I think we should fix this.

continue;
}

connections[i].socket.destroy();
}
};
Expand Down
26 changes: 26 additions & 0 deletions test/parallel/test-http-server-close-idle-wait-response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict';

const common = require('../common');

const { createServer, get } = require('http');

const server = createServer(common.mustCall(function(req, res) {
req.resume();

setTimeout(common.mustCall(() => {
res.writeHead(204, { 'Connection': 'keep-alive', 'Keep-Alive': 'timeout=1' });
res.end();
}), common.platformTimeout(1000));
}));

server.listen(0, function() {
const port = server.address().port;

get(`http://localhost:${port}`, common.mustCall((res) => {
server.close();
})).on('finish', common.mustCall(() => {
setTimeout(common.mustCall(() => {
server.closeIdleConnections();
}), common.platformTimeout(500));
}));
});