fix: close the server after in-flight requests complete

[ndhoule]

Currently, if there are any requests in flight when terminator.terminate is called, terminate will wait for gracefulTerminationTimeout milliseconds, even if all in-flight requests complete before that timeout.

This updates the termination logic in createInternalHttpTerminator to close the server as soon as there are no requests in flight; for example, if the timeout is set to 15000ms but all in-flight requests are complete after 1000ms, terminate will return after ~1000ms.

Fixes #16.

PS: Thanks for maintaining this project! I was led here by of issues with other packages that's pretty much identical to the list in your README.

[ndhoule]

Note that p-wait-for polls for this condition every 1ms. This feels a little janky to me, but was the most straightforward solution I could think of (and clearly represents an improvement over the current behavior). Open to alternative suggestions if this also feels gross to you.

[gajus]

@ndhoule, @danielrearden suggested a possibly better implementation. What do you think?

let resolveSocketsClosed;
const socketsClosed = new Promise((resolve) => {
  resolveSocketsClosed = resolve;
  server.on('connection', (socket) => {
    if (terminating) {
      socket.destroy();
    } else {
      sockets.add(socket);
      socket.once('close', () => {
        sockets.delete(socket);
        if (terminating && !sockets.size && !secureSockets.size) {
          resolve();
        }
      });
    }
  });
  server.on('secureConnection', (socket) => {
    if (terminating) {
      socket.destroy();
    } else {
      secureSockets.add(socket);
      socket.once('close', () => {
        secureSockets.delete(socket);
        if (terminating && !sockets.size && !secureSockets.size) {
          resolve();
        }
      });
    }
  });
});

Then inside terminate

if (!sockets.size && !secureSockets.size) {
  resolveSocketsClosed()
}
await socketsClosed

That should prevent the Promise from hanging around if there were no sockets to start with.

[ndhoule]

@gajus The advantages of the alternative implementation aren't completely obvious; would you mind explaining? (Is it that it avoids polling during termination?)

The alternative seems reasonable to me, though the main tradeoff I see is that the promise resolution logic is a bit more complex to reason about.

[gajus]

Is it that it avoids polling during termination?

Correct.

[eugene1g]

The strategy of await socketsClosed would create a persistent promise that will live for the entire life of the server process. Whereas the "polling" strategy would do polling only during the shutdown phase, which seems less intrusive. I took a similar approach in #17 just without the dependency.

[gajus]

Can we reduce the polling to something more reasonable, like every 10 milliseconds?

Otherwise, Okey to merge.

[eugene1g]

@ndhoule This is a very helpful PR :) Would you be able to change the polling interval to 10m as @gajus suggested - then this might get merged & released soon.

[dwilson6]

I would also like to see this behavior fixed as the docs are currently misleading. Since it seemed like there was some discussion around which approach was preferred, I created a different alternative approach to consider. If you like it, I can put up a PR. I adapted the test @ndhoule added in this PR. I found the t.is(await httpServer.getConnections(), 0); assertion at the end was not sufficiently validating the expected behavior. It also needs to check that the terminate promise has actually resolved.

[ebylund]

Any updates from @ndhoule ? This proposed solution would solve a need, but just wondering if I need to roll with my own solution or wait on this. Happy to offer some help if there is any pending work.

[gajus]

🎉 This PR is included in version 3.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀