-
Notifications
You must be signed in to change notification settings - Fork 22
Closing socket needs to fire a close event. #32
Comments
For UDP this is not needed. When close() is called state is directly "closed". For TCP the steps for close() should not fire an event as the closing handshake is initiated. When clsoing handshake has been completed the close event is fired. TCPServer is similar to UDP. |
See my comment for issue #29. With a close() for UDP, it just makes things more "symmetric" with TCP socket. Would prefer having both "onopen" and "onclose" for UDP, or none. |
If I remember correctly the argument for having "onopen" for UDP was that binding the socket to local interface/port requires some I/O and may take time. I don't think we have a motivation for "onclose". It is a philosophical question of making the API as "symmetric" as possible between UDP and TCP or making the API as simple as possible. I would prefer the latter. If a developer uses the UDP socket API the API shouldn't be more complicated than necessary. |
Binding a UDP socket takes as long as socket() and bind() syscalls to the OS. Both functions will only block for the duration of the call and return success or failure immediately. It is unlike TCP where there is an opening handshake with packets exchanged between peers over network, with time ranging in the milliseconds. So the UDP socket constructor is basically synchronous, just like the UDP socket close() function is. Yes, I end up thinking that you're right about making API as simple as possible. Except making things "symmetric", having "opening" and "closing" ready states for UDP doesn't add any benefit. I would suggest to also drop "opening" ready state for UDP and go directly to "open" if constructor is successful. |
So there is no need for a "close" event for UDP and TCPServer. However, I am still unsure on "open" event for UDP and TCPServer as I have heard different views on the time it can take on binding to a local interface/port. |
Closing and referring to #44. |
The steps to close currently don't fire an event.
The text was updated successfully, but these errors were encountered: