Properly fall back to http.Server when tlsconfig is missing

[Rob--W]

The common pattern in the implementation of Server (including the http.Server and https.Server objects from the Node.js core) is the following (note that this is completely ignored):

if (!(this instanceof Server)) return new Server(...arguments);

There are three relevant Server implementations, with the following inheritance tree:

- tls.Server
 +-- http.Server
 +-- https.Server
   +-- httpolyglot.Server

When httpolyglot.Server is constructed without TLS config, it falls back to constructing itself using http.Server:

// this is an instance of httpolyglot.Server
http.Server.call(this, requestListener);

But this is not an instance of http.Server, so a new http.Server instance is created (and discarded) and this is not modified. So, when listen() is called, the server starts listening on a port
but does not have any request handlers. Consequently, when a http(s) request is sent to the server, the connection is accepted but the request is never processed.

To fix this, be explicit and return the new instance of http.Server (as done in this PR).

[Rob--W]

I've tested the patch with Node v4.2.6 and v7.4.0.

[mscdex]

I think I would rather just get rid of the silent fallback to http.Server and enforce a TLS configuration.

[Rob--W]

I think that the fall-back is useful. I have a project where TLS is optional, so if I cannot read the key and cert, the config stays empty. This is my code: ``` var tlsconfig; try { tlsconfig = { ... with fs.readFileSync ... }; } catch (e) { // ... Show warning with setup instructions... } httpolyglot.createServer(tlsconfig, app).listen(port, callbackWhenListening); ``` If httpolyglot does not support the void tlsconfig option any more, I would need a few more lines to accomplish the same effect (mainly separating the createServer and listen call). If you still want to drop the ability to specify a void tlsconfig, then you have to perform a major version bump (to 0.2.0 would be OK, in semver < 1.0.0 has no guarantee of a stable API).

…

On Jan 8, 2017 1:30 AM, "Brian White" ***@***.***> wrote: I think I would rather just get rid of the silent fallback to http.Server and enforce a TLS configuration. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTUT-UsBwJf3Vsxy5jbrwMsFnZ3HbcHks5rQC4QgaJpZM4Ldi_i> .