chore: close as soon as possible handshake stream#5378
Conversation
| return | ||
| } | ||
|
|
||
| if exists := s.peers.addIfNotExists(stream.Conn(), overlay, i.FullNode); exists { |
There was a problem hiding this comment.
consider (not sure if this is problematic):
if you close the stream earlier before the peer has been added to this peers struct, the other peer will progress with the handshake protocol (continues execution) and may fire a request before the peer was added (the call on this line). i'm not sure if this can cause downstream problems (maybe a protocol already tries to send a message, and some check on the libp2p abstraction that tries to check if the peer exists on this list happens before the peer was added
There was a problem hiding this comment.
Yes, interesting observation. The question is, is handshake stream keeping this synchronization. I am not sure that I can see that by keeping the handshake stream active, other protocols are blocked from sending messages. Am interested to see if I am missing something. Could you elaborate more on your assumptions, or try to see if there could be problems?
I even cannot see why handshake stream is even created earlier than it is needed. This can also be changed.
There was a problem hiding this comment.
iirc the FullClose usage was meant as a means of synchronizing both peers and making sure both peers are at the same stage of the handshake. Previously they would be reading on the stream, waiting for something, and when we would close the stream they could then continue execution.
For example on the connecting side, the protocols start firing here.
The receiver here.
The question is whether under some lock contention on that peer registry or under circumstances will we see one peer finishing the handshake on the other before the other one making it to the line where that peer gets onto the peer registry. If yes then there might be some downstream errors or some weird halfway state (peer is "halfway" connected). Whether the current handshake protocol prevents that fully - can't say, need to have a further look.
|
I have also tried these and the issues from connections_test.go seems to get worse. Investigating this I am getting to a similar opinion as Elad. I am not sure about Closing early. |
|
I am closing the PR as it appears that the stream is kept active in order to synchronize node internals. There is a possibility to improve the code to encapsulate the required operations on both sides, to be more obvious why the stream is kept open, but it is just code maintenance, not a functionality improvement. There is other PR that deals with the improvements of the tests that may be related to this part of the code as well. |
3 participants
Checklist
Description
@gacevicljubisa noticed that handshake.FullClose() can be performed earlier. This PR optimizes and cleans the code related to the handshake stream.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):