Not returning on connection close for chainsync, block-fetch and tx-submission protocol #1141
Conversation
Signed-off-by: Jenita <[email protected]>
Signed-off-by: Jenita <[email protected]>
Signed-off-by: Jenita <[email protected]>
Closes #1112 |
protocol/blockfetch/client.go
Outdated
| c.stateMutex.Lock() | ||
| defer c.stateMutex.Unlock() | ||
| c.currentState = newState | ||
| } |
There was a problem hiding this comment.
The protocol.Protocol base type already tracks the current state. It should be possible to add IsDone() there in a generic way by checking that the current state matches the initial state and/or the current state having AgencyNone.
There was a problem hiding this comment.
updated, could you please review when you get a chance ?
Signed-off-by: Jenita <[email protected]>
Signed-off-by: Jenita <[email protected]>
Signed-off-by: Jenita <[email protected]>
protocol/blockfetch/blockfetch.go
Outdated
| return err | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
These checks need to be done at the top level, in Connection. We have an error handler for muxer errors there, and we can check the state of all active protocols from there.
There was a problem hiding this comment.
made the cahange, could you please review?
Signed-off-by: Jenita <[email protected]>
connection.go
Outdated
| // checkProtocols checks if the protocols are explicitly stopped by the client- treat as normal connection closure | ||
| func (c *Connection) checkProtocols() bool { | ||
| // Check chain-sync protocol | ||
| if c.chainSync != nil && (!c.chainSync.Client.IsDone() || !c.chainSync.Server.IsDone()) { |
There was a problem hiding this comment.
.Client or .Server can be nil, so we need nil checks against both
connection.go
Outdated
| } | ||
|
|
||
| // Check block-fetch protocol | ||
| if c.blockFetch != nil && (!c.blockFetch.Client.IsDone() || !c.blockFetch.Server.IsDone()) { |
There was a problem hiding this comment.
.Client or .Server can be nil, so we need nil checks against both
connection.go
Outdated
| } | ||
|
|
||
| // Check tx-submission protocol | ||
| if c.txSubmission != nil && (!c.txSubmission.Client.IsDone() || !c.txSubmission.Server.IsDone()) { |
There was a problem hiding this comment.
.Client or .Server can be nil, so we need nil checks against both
protocol/blockfetch/client.go
Outdated
| case MessageTypeBatchDone: | ||
| err = c.handleBatchDone() | ||
| case MessageTypeClientDone: | ||
| return nil |
There was a problem hiding this comment.
This doesn't belong here. The client will never receive the ClientDone message, only the server.
protocol/chainsync/client.go
Outdated
| case MessageTypeIntersectNotFound: | ||
| err = c.handleIntersectNotFound(msg) | ||
| case MessageTypeDone: | ||
| return nil |
There was a problem hiding this comment.
This doesn't belong here. The client will never receive this message, only the server
protocol/txsubmission/client.go
Outdated
| case MessageTypeRequestTxs: | ||
| err = c.handleRequestTxs(msg) | ||
| case MessageTypeDone: | ||
| return nil |
There was a problem hiding this comment.
This isn't needed. The client will never receive this message, only the server
Signed-off-by: Jenita <[email protected]>
| // Stop the protocol explicitly | ||
| if err := chainSyncProtocol.Client.Stop(); err != nil { | ||
| t.Fatalf("failed to stop chain sync: %s", err) | ||
| } |
There was a problem hiding this comment.
This isn't really a good way to test this. The protocols aren't meant to be arbitrarily started/stopped from the outside, only from within the actual protocol. Instead, you should use the mock connection to send the Done message to shut it down.
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
I'm not sure that any of this is necessary. The docs for io.ReadFull() and binary.Read() both say that they'll only return io.EOF or io.ErrUnexpectedEOF. We may want to add a check for io.ErrUnexpectedEOF and remap it to io.EOF in the muxer (links below), and then we only need to check for errors.Is(err, io.EOF) anywhere below.
Lines 304 to 306 in a57c097
Lines 331 to 333 in a57c097
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Thinking further about this, we only need to care about the .Server side of each protocol. If we're acting as a client and the connection is unexpectedly closed (we didn't call .Close() ourselves), it should be an error condition.
Closes #1112