socket hang up on HTTPS servers
hildoer opened this issue · 2 comments
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();
}
] );Upon further inspection it looks like the problem is that for https server instances, it fires both 'connection' and 'secureConnection' events. Then, when you call server.stop(), the 'connection' socket is idle even though the 'secureConnection' socket has a single request on it. The loop calling endIfIdle then kills the 'connection' socket, which actually kills the 'secureConnection' socket also.
My guess is that we need to listen to only one of the two events depending on whether its a secure server or not.
Final update... After some digging it looks like 'secureConnection' is fired off by TLS server, not HTTPS. Whereas 'connection' is fired by HTTP server. According to all docs for latest versions of node 4, 6, 8 and 10, a TLS server object will have a function named getTicketKeys. If we test for presence of this function on the server object passed into stoppable, and only do one of the event listens onConnection, this solves the problem.
Let me know if this sounds like a suitable hook in the system. It feels a little hacky but the API for the TLS server object is way stable. I think we can depend on it. If all sounds good I will get a pull request together.