Returning this in net and its inheriting prototype methods

[th507]

One typical way to create a server is through

http.createServer(callback).listen(port);

Then we export the server for other purposes (testing, for example)

module.exports = http.createServer(callback).listen(port);

At this point, everything works as expected.

Later on, when we are trying to add a timeout to the http.Server, we might be tempted to write something like this:

module.exports = http.createServer(callback).listen(port).setTimeout(timeout);

At this time, errors might occur because unlike server.listen, server.setTimeout does not return the instance of http.server.

Event through http module is labelled as stability level 3, but the documentation does not specify the return value for server.listen and for server.setTimeout`. So maybe change the return value is not completely out of the question.

I understand there are multiple setTimeout functions in various modules (like net.setTimeout, message.setTimeout, server.setTimeout, and so on).

Maybe we could consider return http.Server for server.setTimeout(), or even return corresponding instances for all *.setTimeout functions (except the setTimeout in plain JavaScript).

If such changes are not possible, at least we could specify the expected return values for those APIs (namely server.listen, server.setTimeout and some others), so that we might avoid some confusions about those APIs.

[bnoordhuis]

Seems like an okay change to me. For consistency, all setTimeout functions in lib/ (except the global one) should be updated to return this.

One thing to consider is that it's a permanent change. We can't retroactively change the setTimeout functions to return a token like the global setTimeout does, because that would break code that has come to rely on the previous behavior.

[silverwind]

We should investigate various server, net and socket methods. Some are chainable, while many are not.

Like @bnoordhuis said, caution should be taken for cases where it makes sense to return something else.

[yosuke-furukawa]

+1 to @silverwind
We would be better to reconsider what methods should be chainable.

[silverwind]

Looking at just the net.Socket methods, these return undefined right now:

• socket.setKeepAlive()
• socket.setNoDelay()
• socket.setTimeout()
• socket.ref()
• socket.unref()
• socket.destroy()
• socket.end()

[evanlucas]

I'm +1 for returning this on those. I guess my question would be do those need to be in separate PRs?

[silverwind]

Hmm, given the number of methods to change, I'd prefer them in one PR, maybe with separate commits for each method.

We need to be certain that we never intend to return anything else if we go ahead. For example, what if we want to report a failure in a setNoDelay call (maybe to OS doesn't support it)? Do we throw at this point or do we want to return false?

[silverwind]

#1699 looking good so far on setTimeout. I'm thinking maybe we should just extend that PR to include the other methods as well and deliver it all in a semver-minor package?

[silverwind]

One done, seven more socket methods to go. We'll need to check how each method reports errors, but I think they're all potential candidates.

[silverwind]

@evanlucas maybe we can split up the work here. Would you like to add a this return value to setNoDelay and setKeepAlive given your recent involvement in them? I could check out the others.

[evanlucas]

Yes. Was waiting until #1518 got ok'd and landed

[silverwind]

Did socket.setKeepAlive and socket.setNoDelay in #1779. I didn't see much use for doing it in socket.destroy and socket.end as these should be the last call in the chain anyways.

[silverwind]

All done, thank everyone