Fix incorrect docs/disconnect handling in peer_handler #512
Conversation
TheBlueMatt
force-pushed
the
2020-02-peer_handler-docs
branch
from
1f53c04 to
edd732e
Compare
|
Rewrote a ton of docs, and renamed a few functions to clarify what they do. |
TheBlueMatt
force-pushed
the
2020-02-peer_handler-docs
branch
from
edd732e to
6304698
Compare
The way PeerHandler was written, it was supposed to remove from self.peers iff the API docs indicate that disconnect_event should NOT be called (and otherwise rely on disconnect_event to do so). Sadly, the implementation was way out of whack with reality - in the implementation, essentially anywhere where PeerHandler originated the disconnection, the peer was removed and no disconnect_event was expected. The docs, however, indicated that disconnect_event should nearly only be called, only not doing so when the initial handshake message never completed. We opt to change the docs, mostly, as well as clean up the ping/pong handling somewhat and rename a few functions to clarify what they actually do.
TheBlueMatt
force-pushed
the
2020-02-peer_handler-docs
branch
from
6304698 to
faaa4d2
Compare
|
ACK faaa4d2 |
Is this suppose to say "nearly always be called"? |
|
Oops, yea, the commit message is off there. At least the code comments are right :) |
| /// careful to ensure you don't have races whereby you might register a new connection with an fd | ||
| /// the same as a yet-to-be-disconnect_event()-ed. | ||
| /// the same as a yet-to-be-socket_disconnected()-ed. |
There was a problem hiding this comment.
I don't understand what the emphasized is saying:
whereby you might register a new connection with an fd the same as a yet-to-be-socket_disconnected()-ed.
Something seems grammatically off or perhaps I'm just not parsing this correctly.
| pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone { | ||
| /// Attempts to send some data from the given slice to the peer. | ||
| /// | ||
| /// Returns the amount of data which was sent, possibly 0 if the socket has since disconnected. | ||
| /// Note that in the disconnected case, a disconnect_event must still fire and further write | ||
| /// Note that in the disconnected case, a socket_disconnected must still fire and further write |
| /// that event completing. | ||
| /// more calls to write_buffer_space_avail, read_event or socket_disconnected may be made with | ||
| /// this descriptor. No socket_disconnected call should be generated as a result of this call, | ||
| /// though obviously races may occur whereby disconnect_socket is called after a call to |
There was a problem hiding this comment.
Drop "obviously". The reason for including the comment is that this behavior is not obvious.
| /// more calls to write_event, read_event or disconnect_event may be made with this descriptor. | ||
| /// No disconnect_event should be generated as a result of this call, though obviously races | ||
| /// may occur whereby disconnect_socket is called after a call to disconnect_event but prior to | ||
| /// that event completing. | ||
| /// more calls to write_buffer_space_avail, read_event or socket_disconnected may be made with | ||
| /// this descriptor. No socket_disconnected call should be generated as a result of this call, | ||
| /// though obviously races may occur whereby disconnect_socket is called after a call to | ||
| /// socket_disconnected but prior to that event completing. |
There was a problem hiding this comment.
"No disconnect_event" was changed to "No socket_disconnected call" here but later in the comment "event" is still used.
prior to that event completing
Rather than replacing "event" with "call", I'd recommend re-wording to something a little less ambiguous such as:
prior to the former completing
| /// generate no further read_event/write_buffer_space_avail calls for the descriptor, only | ||
| /// triggering a single socket_disconnected call (unless it was provided in response to a | ||
| /// new_*_connection event, in which case no such socket_disconnected() must be generated and the | ||
| /// socket be silently disconencted). |
| /// but must NOT be called if a PeerHandleError was provided out of a new_\*\_connection event! | ||
| /// This must only be called if the socket has been disconnected by the peer or your own | ||
| /// decision to disconnect it and must NOT be called in any case where other parts of this | ||
| /// library (eg PeerHandleError, explicit disconnect_socket calls) instruct you to disconnect |
There was a problem hiding this comment.
How is the user supposed to differentiate between their own call to disconnect_socket and that of this library?
There was a problem hiding this comment.
Hmm, disconnect_socket is different from socket_disconnected - I suppose a user could call disconnect_socket on their own Descriptor, but in general I presumed they wouldn't (and would just call shutdown(2)).
Oops, forgot I assigned you on this one before merging, in any case, not really a big deal either way. Post-merge review is 98% as good as pre-merge review IMO :) |
The way PeerHandler was written, it was supposed to remove from
self.peers iff the API docs indicate that disconnect_event should
NOT be called (and otherwise rely on disconnect_event to do so).
Sadly, the implementation was way out of whack with reality - in
the implementation, essentially anywhere where PeerHandler
originated the disconnection, the peer was removed and no
disconnect_event was expected. The docs, however, indicated that
disconnect_event should nearly only be called, only not doing so
when the initial handshake message never completed.
We opt to change the docs, mostly, as well as clean up the
ping/pong handling somewhat.
Frankly I'm not sure if lightning-net-tokio complies with this properly, but I'm gonna update #472 with the correct behavior in a sec.