Standardize ready event for built-in streams

[addaleax]

Currently, various internal streams have different events that indicate that the underlying resource has successfully been established:

• fs streams use the open event once the file descriptor is available
• net.Sockets use the connect event once the socket has been established
• Http2Streams use the ready event once the underlying http/2 session is established

I would like to suggest standardizing on emitting ready for all of these streams, i.e. emitting ready for fs streams and network sockets in addition to the event names they currently use.

If there are no objections, I think this makes for a good first contribution.

[mscdex]

I've seen 'ready' used in third party modules (including my own -- although not on core objects directly), so you might be careful about using the event name in case someone is emitting it for their own signalling on TCP sockets for example.

[ryzokuken]

@addaleax would this just involve renaming the event being emitted to ready in the two cases?

[addaleax]

@mscdex Do you have any suggestions? Should we just try it, see if something breaks, and otherwise rename?

@ryzokuken It would mean adding the event in the places where the other events are currently being emitted. I think renaming them, including removing the old names, would be too much of a breaking change.

[ryzokuken]

Agreed. Emitting an extra event sounds like a much safer approach.

…

On Tue 13 Mar, 2018, 2:32 AM Anna Henningsen, ***@***.***> wrote: @mscdex <https://github.com/mscdex> Do you have any suggestions? Should we just try it, see if something breaks, and otherwise rename? @ryzokuken <https://github.com/ryzokuken> It would mean adding the event in the places where the other events are currently being emitted. I think renaming them, including removing the old names, would be too much of a breaking change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMg3MvTa5br9BW0JHxLjDBz8h_GHaTQzks5tduJigaJpZM4SnOfl> .

[mscdex]

@addaleax perhaps @ChALkeR can search for the string literal in the npm ecosystem and see what turns up?

I'm not 100% convinced this kind of change is really needed though.

[addaleax]

I'm not 100% convinced this kind of change is really needed though.

It’s going to be helpful in merging streams code between the different implementations.

[addaleax]

Actually, it would also be nice to have something like the corresponding boolean property, which is .connecting for Sockets and .pending for http2 streams, available for all of these consistently.

[mscdex]

I think the term 'ready' can be misleading and can depend on the object emitting the event.

For example, if you make a TLS connection, what does "ready" mean? That the underlying TCP connection was established or that the TLS handshake completed successfully? Do we then have to still have distinguishing events, like 'ready' and 'secureReady' ? Do we change how TLS sockets work so they no longer emit 'connect' but only 'ready' once the handshake completes (but "plain" TCP connections will emit 'ready')?

I just don't see the value in potentially adding more confusion by using a subjective term in core (I haven't paid attention to http/2 support, but I am surprised it is currently using such an event name).

[addaleax]

I think the term 'ready' can be misleading and can depend on the object emitting the event.

We can bikeshed over the name. streamReady works for me too, but I don’t think the name chosen for http2 is inapt. edit: or streamEstablished?

That the underlying TCP connection was established or that the TLS handshake completed successfully?

The latter.

Do we then have to still have distinguishing events, like 'ready' and 'secureReady' ?

No.

Do we change how TLS sockets work so they no longer emit 'connect' but only 'ready' once the handshake completes (but "plain" TCP connections will emit 'ready')?

No.

I just don't see the value in potentially adding more confusion by using a subjective term in core (I haven't paid attention to http/2 support, but I am surprised it is currently using such an event name).

You are conflating two things here: The naming of the event (which can be subjective, granted) and whether we should have a single, consistent event name used for these streams.

As mentioned above, I’m okay with another name; the semantics are “this is the point from where reading and writing data actually works”. I think ready would describe that, but I’d also be completely fine with a more verbose name. (I think we can just change the name on the http/2 stream class if we wanted to.)

I do see value in having a single, consistent event name: Partly because it makes our APIs more consistent, which seems like an inherently good thing to me, but mostly because it would actually simplify re-using code between these implementations. (I opened this issue specifically because of that.)

[j0t3x]

I would like to take this one 😄

[addaleax]

@j0t3x I assume you have read the discussion here – if you want to open a PR, feel free to go for it. :) Let us know if you have any questions!

[jvelezpo]

I will take some time this night to work on it

[sameer-coder]

@addaleax Anna,
I have opened a PR for this. Would appreciate if you can take a look and provide feedback.
Included tests as well.