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

http.server.close() closes idle connections since 18.19.0 #51677

Closed
Llois41 opened this issue Feb 6, 2024 · 5 comments
Closed

http.server.close() closes idle connections since 18.19.0 #51677

Llois41 opened this issue Feb 6, 2024 · 5 comments
Labels
http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Comments

@Llois41
Copy link

Llois41 commented Feb 6, 2024

Version

18.19.0

Platform

Linux, Mac, Windows

Subsystem

No response

What steps will reproduce the bug?

import http from 'node:http';
import timers from 'node:timers/promises';

const server = new http.Server((req, res) => res.end());
server.keepAliveTimeout = 5_000;
server.listen(8080);

const agent = new http.Agent({ keepAlive: true, keepAliveMsecs: 60_000 });
await get();
console.log('request 1 completed');

await timers.setTimeout(1_000);

const closed = new Promise((resolve) => server.close(resolve));
console.log('listening socket closed');

await get();
console.log('request 2 completed');

await closed;
console.log('socket completely closed');

function get() {
  return new Promise((resolve, reject) => {
    http.get('http://localhost:8080', { agent }, (res) => {
      res.resume();
      if (res.statusCode !== 200) {
        reject(new Error(`request failed with status code ${res.statusCode}`));
      } else {
        resolve(undefined);
      }
    });
  });
}

Works as expected with Node 18.18.2:

$ node http-close.mjs
request 1 completed
listening socket closed
request 2 completed
socket completely closed

Second request fails on Node 18.19.0 because all idle connections are closed:

$ node http-close.mjs
request 1 completed
listening socket closed
node:events:495
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at connResetException (node:internal/errors:720:14)
    at Socket.socketOnEnd (node:_http_client:525:23)
    at Socket.emit (node:events:529:35)
    at endReadableNT (node:internal/streams/readable:1400:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
Emitted 'error' event on ClientRequest instance at:
    at Socket.socketOnEnd (node:_http_client:525:9)
    at Socket.emit (node:events:529:35)
    at endReadableNT (node:internal/streams/readable:1400:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'ECONNRESET'
}

How often does it reproduce? Is there a required condition?

We upgraded to Node 18.19.0 (from 18.18.2) and saw our test fail that tests graceful shutdown. This tests sends a request before (with keepalive set) and after the http.Server is closed to ensure that all requests will be answered and there will be no HTTP or socket errors.

In the docs it is also explicitly stated that the http.server.close() only stops the server from accepting new connections but not terminating open ones.

What is the expected behavior? Why is that the expected behavior?

The expected behavior ist that calling http.server.close() does not terminate idle connections. Maybe closing (idle) connections should be configurable via an options parameter.

What do you see instead?

http.server.close() calls http.server.closeIdleConnections() which will terminate all idle connections.

Additional information

We think we found the commit that introduced this behavior.

Commit on main: bd7a808 (only extracts a function)

Commit on tag 18.19.0: 2969722 (adds the .closeIdleConnections() call)

@climba03003
Copy link
Contributor

The commit in main branch is correct, but the backport is not.
#50194

@Llois41 Llois41 changed the title http.server.close() closes open connections since 18.19.0 http.server.close() closes idle connections since 18.19.0 Feb 6, 2024
@VoltrexKeyva VoltrexKeyva added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Feb 8, 2024
@kumarrishav
Copy link
Contributor

seems like it's same as. #52330

@kumarrishav
Copy link
Contributor

CC: @bnoordhuis

kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
Correcting the nodejs#50194 backporting mistake. server.closeIdleConnections shouldn't be called while server.close in node v18. This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677,
kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
Correcting the nodejs#50194 backporting mistake.
closeIdleConnections shouldn't be called while server.close in node v18.
This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677,
kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
….\n Correcting the nodejs#50194 backporting mistake.\n closeIdleConnections shouldnot be called while server.close in node v18.\n This behavior is for node v19 and above.\n fixes: nodejs#52330, nodejs#51677
kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
Correcting the nodejs#50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677
kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
Correcting the nodejs#50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677
kumarrishav added a commit to kumarrishav/node that referenced this issue Apr 2, 2024
Correcting the nodejs#50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677
richardlau pushed a commit that referenced this issue Apr 17, 2024
Correcting the #50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

Fixes: #52330
Fixes: #51677
PR-URL: #52336
Refs: #50194
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
@michael42
Copy link

Awesome, thanks for the fix that will presumably be released with the next Node 18 version. 🎉

Does anyone know if/how one can implement a robust graceful shutdown with Node 19+? Right now we're doing something like:

const server = new http.Server(...);
server.closeIdleConnections = () => undefined;

to avoid server.close() closing idle connections and we're looking for something more robust/supported/documented.

@richardlau
Copy link
Member

This should be fixed in Node.js 18.20.3.

@richardlau richardlau added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

No branches or pull requests

6 participants