Merge pull request #85 from blushingpenguin/node-timeout

disable keep alive timeout, headers timeout and log any server timeouts

Author: JeremyTCD

Changelog.md

@@ -3,7 +3,11 @@ This project uses [semantic versioning](http://semver.org/spec/v2.0.0.html). Ref
 *[Semantic Versioning in Practice](https://www.jering.tech/articles/semantic-versioning-in-practice)*
 for an overview of semantic versioning.
 
-## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/5.4.1...HEAD)
+## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/5.4.2...HEAD)
+
+## [5.4.2](https://github.com/JeringTech/Javascript.NodeJS/compare/5.4.1...5.4.2) - Jun 25, 2020
+### Fixes
+- Disabled unecessary Node.js HTTP timeouts, added logging for timeouts. ([#85](https://github.com/JeringTech/Javascript.NodeJS/pull/85)).
 
 ## [5.4.1](https://github.com/JeringTech/Javascript.NodeJS/compare/5.4.0...5.4.1) - Jun 23, 2020
 ### Fixes

src/NodeJS/Javascript/Servers/OutOfProcess/Http/HttpServer.ts

@@ -3,7 +3,7 @@ var Module = require('module');
 import * as path from 'path';
 import * as http from 'http';
 import * as stream from 'stream';
-import { AddressInfo } from 'net';
+import { AddressInfo, Socket } from 'net';
 import InvocationRequest from '../../../InvocationData/InvocationRequest';
 import ModuleSourceType from '../../../InvocationData/ModuleSourceType';
 
@@ -26,9 +26,29 @@ let projectDir = process.cwd();
 // Create server
 const server = http.createServer(serverOnRequestListener);
 
-// In Node.js v13+ this is the default, however for earlier versions it is 120 seconds.
+// The timeouts below are designed to manage network instability. Since we're using the HTTP protocol on a local machine, we can disable them
+// to avoid their overhead and stability issues.
+
+// By default, on older versions of Node.js, request handling times out after 120 seconds.
+// This timeout is disabled by default on Node.js v13+. 
+// Becuase of the older versions, we explicitly disable it.
 server.setTimeout(0);
 
+// By default, a socket is destroyed if it receives no incoming data for 5 seconds: https://nodejs.org/api/http.html#http_server_keepalivetimeout. 
+// This is good practice when making external requests because DNS records may change: https://github.com/dotnet/runtime/issues/18348.
+// Since we're using the HTTP protocol on a local machine, it's safe and more efficient to keep sockets alive indefinitely.
+server.keepAliveTimeout = 0;
+
+// By default, a socket is destroyed if its incoming headers take longer than 60 seconds: https://nodejs.org/api/http.html#http_server_headerstimeout.
+// In early versions of Node.js, even if setTimeout() was specified with a non-zero value, the server would wait indefinitely for headers. 
+// This timeout was added to deal with that issue. We specify setTimeout(0), so this timeout is of no use to us.
+//
+// Note that while 0 disables this timeout in node 12.17+, in earlier versions it causes requests to time out immediately, so set to 10 years.
+server.headersTimeout = 10 * 365 * 24 * 60 * 60 * 1000;
+
+// Log timed out connections for debugging
+server.on('timeout', serverOnTimeout);
+
 // Send client error details to client for debugging
 server.on('clientError', serverOnClientError);
 
@@ -182,6 +202,12 @@ function serverOnClientError(error: Error, socket: stream.Duplex) {
     socket.end(httpResponseMessage);
 }
 
+// Send timeout details to client for debugging - this shouldn't fire but there have been various node http server timeout issues in the past.
+// The socket won't actually get closed (the timeout function needs to do that manually).
+function serverOnTimeout(socket: Socket) {
+    console.error(`Ignoring unexpected socket timeout for address ${socket.remoteAddress}, port ${socket.remotePort}`);
+}
+
 function serverOnListeningListener() {
     // Signal to HttpNodeHost which loopback IP address (IPv4 or IPv6) and port it should make its HTTP connections on
     // and that we are ready to process invocations.