net: use actual Timeout instances on Sockets

[Fishrock123]

This makes net.Sockets use actual Timeout objects in a _timeout property, rather than making the socket itself a timer and appending properties to it directly.

This should make the code generally easier to understand, and might also prevent some deopts from properties being changes on the socket itself.

It is possible this could effect performance either better or worse, but I highly doubt it would be a significant difference. I'll try to run benchmarks on it in the coming days, but if anyone else would like to that would also be appreciated.

This also exposes timers.Timeout so as to avoid a circular dependency with the new lib/internal/timers.js, but we might not want that so I should be able to refactor more of timers into the internals file if necessary. Tagging as semver-minor for now due to this. I could also split that into separate commits.

Made live on https://twitch.tv/nodesource

CI: https://ci.nodejs.org/job/node-test-pull-request/6202/

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

net, timers

[cjihrig]

Can you remove the commented code.

[cjihrig]

Maybe a result of the commented code, but this could be

if (s._timeout)
timers._unrefActive(s._timeout);

[Fishrock123]

Ah right, forgot to remove that before PRing. Updated, new CI: https://ci.nodejs.org/job/node-test-pull-request/6203/

[jasnell]

Rather than introducing yet another _ prefixed property, I'd much prefer to use a Symbol.
Also, given that I'm following an identical pattern in the http2 work, I'd like that Symbol to be reusable (perhaps as an export off the internal/timers.js this PR creates

[evanlucas]

Isn't that part of the implementation of a Timeout instance? i.e. didn't a socket already have it, it just wasn't declared in the constructor?

[jasnell]

Not that I'm able to see. There are zero hits anywhere in the source for a _timeout property

[evanlucas]

It gets added if Socket#setTimeout() is called iirc https://github.com/nodejs/node/blob/master/lib/net.js#L330

[jasnell]

Again, not that I'm able to see: https://github.com/nodejs/node/blob/master/lib/timers.js#L296-L317

[evanlucas]

Apologies, I was thinking of ._idleTimeout. Disregard

[Fishrock123]

It should be noted that in situations where timer properties were previously available on the Socket object, they will not be after this patch. I highly doubt people rely on those though, since they may not always be there to begin with.

[addaleax]

Can timers.js re-use this value by importing it from here?

[lpinca]

Isn't this semver-major?

[jasnell]

I think I would lean more towards semver-major on this, just to be careful

[evanlucas]

+1 to Semver-major

[Fishrock123]

Theoretically semver-major yes. If people seem to think it should be then so be it, I don't hold strongly one way or other.

[Fishrock123]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/571/

[Fishrock123]

Addressed most of the remaining comments.

@jasnell I made the new property hidden using a kTimeout symbol. I'm slightly worried if anyone still needed to access the timeout directly on a socket after this change. Maybe we could just expose a new API if that was really necessary, or perhaps the existing APIs already cover anything that could be needed.

[jasnell]

We should be safe but if someone really needed that access then adding a new API shouldn't be out of the question.

[jasnell]

@Fishrock123 ... ping... what's the status on this one?

[Fishrock123]

Ugh, I'm just worried that we will end up breaking some module or something.

Would anyone be calling e.g. timers.active() directly on a Socket? timers.unenroll()? I mean I highly doubt it so probably not but who knows?

... new CI: https://ci.nodejs.org/job/node-test-pull-request/6985/

Guess we might as well land if it looks good. @nodejs/ctc if anyone else has significant reservations.

[mscdex]

Can the new internal module use the module.exports = {} format for consistency?

[Fishrock123]

@mscdex Done, updated and rebased.

CI: https://ci.nodejs.org/job/node-test-pull-request/6987/

[mscdex]

I prefer 'positive, finite number'. I recall having a similar discussion in a previous PR about using 'non-negative' vs 'positive' in error messages. To me it sounds more straight-forward.

As far as including the original value in the error message goes, I don't know if we've "standardized" on how it's presented, but to me the current wording is a little awkward. Perhaps it could be more explicit, like

'... must be a positive, finite number. Saw value: '+msecs

or

'... must be a positive, finite number. Received value: '+msecs

[bnoordhuis]

Zero is an allowed value but is not a positive number, though.

[mscdex]

Zero can be positive or negative, at least as far as Javascript is concerned.

[mscdex]

FWIW here was the previous PR I had in mind where "non-negative" was dropped in favor of "positive": #10769

[Fishrock123]

@mscdex Does any of this really matter, since it is internal anyways?

[mscdex]

I think to check the performance difference of these changes, we'd need to either create a new benchmark or possibly run one of the http benchmarks (since timeouts may be used there) because those in benchmarks/net do not exercise socket.setTimeout().

[mscdex]

@Fishrock123 What do you mean "any of this?"

[mscdex]

Just to add, just because this function is internal does not mean suggestions should be ignored, especially because functions like setUnrefTimeout() are both used during runtime and are passed user values like msecs.

[BridgeAR]

@Fishrock123 this needs a rebase

[BridgeAR]

I am closing this due to long inactivity and no response.

@Fishrock123 please reopen if you would like to follow up on this.

[Fishrock123]

This is something I am still interested in pursuing in the very near future.

[Fishrock123]

Still very busy but on my short-term todo list

[BridgeAR]

@Fishrock123 I did not fully check if all comments were addressed or if you just rebased. Could you have another look? :-)

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/11626/

[Fishrock123]

FUN FACT: this patch still needed all those internal checks!

...

[Fishrock123]

This should fail: https://ci.nodejs.org/job/node-test-pull-request/11650/

I can't get these to pass, I'm not sure why a clientError is emitted (this was not a problem when the patch was first ready to land).

Edit: to clarify, it fails to parse the request chunk... somehow.

=== release test-http-server-keep-alive-timeout-slow-client-headers ===
Path: sequential/test-http-server-keep-alive-timeout-slow-client-headers
'HTTP/1.1 200 OK\r\nDate: Thu, 23 Nov 2017 01:12:46 GMT\r\nConnection: keep-alive\r\nContent-Length: 0\r\n\r\n'
{ Error: Parse Error
at socketOnEnd (_http_server.js:424:20)
at Socket.emit (events.js:164:20)
at endReadableNT (_stream_readable.js:1062:12)
at _combinedTickCallback (internal/process/next_tick.js:137:11)
at process._tickCallback (internal/process/next_tick.js:179:9) bytesParsed: 0, code: 'HPE_INVALID_EOF_STATE' }
events.js:136
throw er; // Unhandled 'error' event
^
Error [ERR_STREAM_WRITE_AFTER_END]: write after end
at writeAfterEnd (_stream_writable.js:237:12)
at Socket.Writable.write (_stream_writable.js:288:5)
at Socket.write (net.js:739:40)
at Timeout.setTimeout [as _onTimeout] (/Users/Jeremiah/Documents/node/test/sequential/test-http-server-keep-alive-timeout-slow-client-headers.js:41:14)
at ontimeout (timers.js:489:11)
at tryOnTimeout (timers.js:313:5)
at Timer.listOnTimeout (timers.js:273:5)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/sequential/test-http-server-keep-alive-timeout-slow-client-headers.js
=== release test-http-server-keep-alive-timeout-slow-server ===
Path: sequential/test-http-server-keep-alive-timeout-slow-server
events.js:136
throw er; // Unhandled 'error' event
^
Error: socket hang up
at createHangUpError (_http_client.js:330:15)
at Socket.socketOnEnd (_http_client.js:423:23)
at Socket.emit (events.js:164:20)
at endReadableNT (_stream_readable.js:1062:12)
at _combinedTickCallback (internal/process/next_tick.js:137:11)
at process._tickCallback (internal/process/next_tick.js:179:9)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/sequential/test-http-server-keep-alive-timeout-slow-server.js

cc @nodejs/http

[Fishrock123]

All type checking done in this patch is necessary for consistency with other parts of the code. If you want it changed open a separate PR.

[Fishrock123]

Just gona open a new PR.