p2p: add MemoryTransport, an in-memory transport for testing#5827
p2p: add MemoryTransport, an in-memory transport for testing#5827erikgrinaker merged 1 commit intomasterfrom
Conversation
422205e to
87f775b
Compare
Codecov Report
@@ Coverage Diff @@
## master #5827 +/- ##
==========================================
- Coverage 59.90% 59.72% -0.19%
==========================================
Files 262 263 +1
Lines 23688 23834 +146
==========================================
+ Hits 14190 14234 +44
- Misses 7991 8073 +82
- Partials 1507 1527 +20
|
| // check close first, since channels are buffered | ||
| select { | ||
| case <-c.closeCh: | ||
| return 0, nil, io.EOF | ||
| default: | ||
| } |
There was a problem hiding this comment.
Why do we need to do this? The go c.close can be closed between this block and the next select.
There was a problem hiding this comment.
This is to make sure that if the connection is already closed, we never receive or send a message. For example, if receiveCh contains a buffered message but the connection has been closed, without this check there is a 50% chance that the buffered message will be returned even though the connection has been closed.
If the connection is closed after this check (concurrently with the select) there is also a 50% chance that the message will be returned, but that is consistent with concurrency semantics where either the receive event or the close event may happen first depending on goroutine/channel scheduling. However, after a close event there should never be a receive event.
There was a problem hiding this comment.
That makes sense, but I don't see what first select buys us since there is still a chance we receive and return a message if the connection is closed before the last select.
There was a problem hiding this comment.
Without the select, a single (non-concurrent) client can see this behavior:
connection.Close()
msg := connection.ReceiveMessage() // returns a message!This is clearly wrong. The first select makes sure this will never happen.
If they are concurrent, however, it is entirely reasonable for there to be a 50% chance of receiving a message:
go func() {
connection.Close()
}()
go func() {
msg := connection.ReceiveMessage() // 50% chance of returning, entirely reasonable
}()The first select is necessary to make connection closures linearizable, i.e. once the connection is closed it stays closed.
There was a problem hiding this comment.
Ok, I think that's reasonable 👍
This should probably have more tests, but I expect the transport layer to change quite a bit before the P2P refactor is done, so we'll add more later.