I am seeing an issue where stoppable is killing in-flight HTTPS connections. My test verification sets up an HTTPS server, applies stoppable, makes a request (the handler will delay 1 second to reply), calls server.stop() immediately without waiting for the request to return, and the request then errors out immediately with "socket hang up" error.

However, if I use HTTP instead, and leave all other code the same, everything works correctly and the server.stop() command waits the 1 seconds for the delay handler to return, sees the connection ended, then the stop() callback fires.

I have also tested with keep alive enabled and disabled on the request side of things. In both cases HTTP works, and HTTPS gets a socket hang up.

Here is my test file. To run it, install async, request and stoppable, set the "configure test here" values, and then run the file with node v6 or v8. For config, the problems occur for secure = true, no matter what the other two config values are.

Keep in mind, if you use the config values of applyStoppable = false and keepAlive = true, the server will hang forever. In that case ctrl+c to exit.

const async = require( 'async' ); // https://www.npmjs.com/package/async
const fs = require( 'fs' ); // native
const http = require( 'http' ); // native
const https = require( 'https' ); // native
const request = require( 'request' ); // https://www.npmjs.com/package/request
const stoppable = require( 'stoppable' ); // https://www.npmjs.com/package/stoppable

// configure test here
let secure = true; // run tests on HTTP (false) or HTTPS w/ self-signed cert (true)
let applyStoppable = true; // run native server with close (false) or stoppable server with stop (true)
let keepAlive = false; // whether or not to use keep alive agent for requests

/**
 * Generated the self-signed cert in certs directory with:
 *
 * openssl genrsa -out test.key 2048
 * openssl req -new -key test.key -out test.csr
 * openssl x509 -req -in test.csr -signkey test.key -out test.crt -days 3650 -sha256
 *
 */
let tlsOptions = {
  key: fs.existsSync( 'certs/test.key' ) ?
       fs.readFileSync( 'certs/test.key', 'utf8' ) :
       null,
  cert: fs.existsSync( 'certs/test.crt' ) ?
        fs.readFileSync( 'certs/test.crt', 'utf8' ) :
        null
};

// a logging utility that prepends timing info
let log = function ( message ) {
  console.log( new Date().toISOString(), message );
};

let onRequestHandler = null; // this gets set on line 92

// echo back
let requestHandler = function ( req, res ) {

  // lets the control code know the request has connected and we can
  // call server.stop()
  if ( typeof onRequestHandler === 'function' ) {
    let temp = onRequestHandler;
    onRequestHandler = null;
    temp();
  }

  // make up a response payload
  let response = {};

  response.method = req.method;
  response.url = req.url;
  response.headers = req.headers;

  log( 'request received, delaying response: 1000ms' );

  setTimeout( () => {

    // after we delay 1000ms, allowing time for the server.stop() command to be called, send the response
    res.writeHead( 200, { 'Content-Type': 'application/json' } );
    res.write( JSON.stringify( response ) );
    res.end();

    log( 'response sent' );

  }, 1000 );

};

// select HTTP or HTTPS server
let server = secure ?
             https.createServer( tlsOptions, requestHandler ) :
             http.createServer( requestHandler );

server.keepAliveTimeout = 0; // make keep alive connections hang for always (makes node v8+ act like node v6)

log( 'testing HTTP' + (secure ? 'S' : '') + ' protocol ' +
     'w/' + (applyStoppable ? '' : 'out') + ' stoppable ' +
     'on ' + (keepAlive ? '' : 'non-') + 'keep-alive connection' );

if ( applyStoppable ) {
  server = stoppable( server, 30000 ); // no connection with an active HTTP request should be closed for at least 30s
}

async.series( [
  function ( done ) {
    server.listen( secure ? 8443 : 8080, done ); // start the server listening
  },
  function ( done ) {

    // fire the call back as soon as the request handler starts handling the request about to be sent, but before the response to the request is returned
    onRequestHandler = done;

    // start a request to the delay handler
    request( {
      url: 'http' + (secure ? 's' : '') + '://localhost:' + (secure ? 8443 : 8080),
      agentOptions: {
        keepAlive: keepAlive,
        rejectUnauthorized: false // allow self-signed cert
      }
    }, function ( err ) {
      log( 'request err: ' + err ); // this should never error because the response comes in long before grace period
    } );

  },
  function ( done ) {

    log( 'stop server sent' );
    if ( applyStoppable ) {

      // call stop(), since we are using stoppable
      server.stop( done );
    } else {

      // use native close(), since we are not using stoppable
      server.close( done );
    }

  },
  function ( done ) {
    log( 'stop server complete' ); // all connections are kills and stop() or close() fired their callback
    done();
  }
] );

I was banging my head trying to figure out why my Node 12.8.3 process' memory footprint was increasing indefinitely and managed to narrow down the problem to this module (used by @godaddy/Terminus in my case).

For some reason, the server._pendingSockets map would not be properly maintained, keeping references to thousands of requests.

As soon as I switched from using stoppable to http-terminator, the problem went away.

Please note that I am using node-spdy for the http2 server, since the core implementation is not yet supported by express.

I've noticed that you're creating a property on every socket. I believe that would cause all these socket objects to be deoptimized. A possible solution that shouldn't impact much I believe would be to change your Set to a Map (also has forEach), and use sockets.set(socket, 0);, the 0 being the number of pending requests.

Thoughts?

I'm curious whether folks are interested in revisiting some of the design decisions.

For one it'd be interesting if stoppable() could return the stop() function, rather than assigning it to the server value. Calling code could still do server.stop = stoppable(server). Personally I don't find the chaining argument convincing.

It'd also be neat if stop() would return a promise, rather than taking a callback. It wouldn't match the server.close() API but in these glorious days of async / await I think returning a promise would be more usable.

Once stop() no longer takes a callback it'd be possible to pass grace directly.

These last two changes combined make it possible to use stoppable with https://www.npmjs.com/package/@digicat/termination-manager (which, full disclaimer, I wrote). That library can vary the grace period based on whether the process is crashing or merely terminating, which doesn't work with stoppable as-is.

If folks disagree I'm happy to publish a fork elsewhere but I figured I'd ask first. These nits aside stoppable is the nicest library I've found for stopping servers 🥇

Thanks for reading this far!

Hi, great work on this module, I like its minimalism and simplicity.

While analyzing the code I noticed that if all in-flight requests finish before the grace period is over, we still wait for the grace timeout to finish. That goes a bit counter to how I would want to setup my HTTP server: a relatively long grace period for big downloads or slow clients, but within reason. Say I am willing to wait up to 30 seconds. If it's relatively quiet on my server, stoppable will make me wait the full 30 seconds, even if a single request was in flight for an extra millisecond when stop was called.

One solution could be to keep a global connection counter around and clearTimeout if it hits 0 during the grace timer.

Thoughts?

i don't see socket hang up errors being handled :)

Why cant we have a function that keps on checking remaining sockets and close them. It will be called in stop().

`
function stop (callback) {

// allow request handlers to update state before we act on that state

setImmediate(() => {

  stopped = true

  if (grace < Infinity) {

    setTimeout(destroyAll, grace).unref()

  }

  server.close(e => {

    if (callback) {

      callback(e, gracefully)

    }

  })

  reqsPerSocket.forEach(endIfIdle)
   if (!grace)
      closeRemaining(reqsPerSocket);
})

}
function closeRemaining(reqsPerSocket){
if(reqsPerSocket.size>0)
{
reqsPerSocket.forEach(endIfIdle);
if(reqsPerSocket.size>0){
setTimeout(closeRemaining, 1000)
}
}
}

`

Node 6 is currently LTS. Can we make this Node 6 compatible?

This is the express, how can I wrap it around stoppable?

const app = require('./src/app');

const port = process.env.PORT || 3000;
app.listen(port, () => {
  console.log(`Listening on port ${port}...`);
});

1. https://github.com/hunterloftis/stoppable/blob/master/lib/stoppable.js#L35

server.close should be called before setImmediate, so it happens as soon as possible.
and "stopped = true" should be before setImmediate - so all the requests that complete in current I/O phase would already see that shutdown is initiated.

2. https://github.com/hunterloftis/stoppable/blob/master/lib/stoppable.js#L46

By the time socket.destroy() is called, there might have been already new connections in I/O phase. And since destroy() is called in the end of I/O phase then possibly it is destroying currently active socket with active request. Simply removing the setImmediate and calling socket.destroy() right after socket.end() call should be sufficient.

3. https://github.com/hunterloftis/stoppable/blob/master/lib/stoppable.js#L23 -- after socket.end() should be socket.destory() call.

Hi, This module seems to do everything the correct way but I wonder would it be more correct to also send the "Connection: Close" header to the client on the last inflight request before closing the socket? Otherwise we will end "Connection: Keep-Alive" back and the client will not be expecting the socket to close.

i.e. the logic would go here: https://github.com/hunterloftis/stoppable/blob/master/lib/stoppable.js#L27

How do i use it with cluster module. I would like to have multiple workers and restart them indivudially.

This is ideally done with worker.disconnect();

I have answered a variation of "how to terminate a HTTP server" many times on different Node.js support channels. Unfortunately, I couldn't recommend any of the existing libraries because they are lacking in one or another way (https://github.com/gajus/http-terminator#alternative-libraries). I have since put together a package that (I believe) is handling all the cases expected of graceful HTTP termination.

https://github.com/gajus/http-terminator

I would appreciate if you consider contributing to http-terminator and deprecating stoppable.

http-shutdown seems to do exactly what this package does

https://github.com/thedillonb/http-shutdown

Can you offer a comparison of the features?

Hi!
I have some problems with using stoppable on node v8.7.0

here are couple of stack traces:

    TypeError: setTimeout(...).unref is not a function

      at Immediate.setImmediate [as _onImmediate] (node_modules/stoppable/lib/stoppable.js:33:39)
      at runCallback (timers.js:785:20)
      at tryOnImmediate (timers.js:747:5)
      at processImmediate [as _immediateCallback] (timers.js:718:5)

    TypeError: setTimeout(...).unref is not a function

      at Immediate.setImmediate [as _onImmediate] (node_modules/stoppable/lib/stoppable.js:33:39)
      at runCallback (timers.js:785:20)
      at tryOnImmediate (timers.js:747:5)
      at processImmediate (timers.js:718:5)
      at _combinedTickCallback (internal/process/next_tick.js:131:7)
      at process._tickCallback (internal/process/next_tick.js:180:9)

I use babel, jest, flow and lots of other things, so it's hard to say is it problem of stoppable or not.