search

haskell-distributed / network-transport-tcp

TCP Realisation of Network.Transport
http://haskell-distributed.github.com
BSD 3-Clause "New" or "Revised" License
30 stars 27 forks source link

Use equality rather than ordering in socket close #56

Closed avieth closed 7 years ago

avieth commented 7 years ago

Given a pair of endpoints, each of them knows

In order to close the socket, the peers must agree on these: locally- created of A must match remotely-created of B, and remotely-created of A must match locally-created of B.

When A sends CloseSocket, it can be sure that when it reaches B, B will have seen all of A's locally-created connections (property of TCP), so locally-created of A certainly matches remotely-created of B. To verify the other half, A sends its latest observed remotely-created connection identifier, and B just has to check that it matches its latest locally-created identifier. There's no need to use ordering, equality is enough.

The protocol is now more liberal: you don't have to allocate connection identifiers monotonically, you just have to ensure that they have a sufficiently long period. A period of n means that n in-flight CreatedNewConnection messages would give a false positive on the equality test.

The check for outgoing connections is still present, but it only detects a protocol error by the peer. If they agree on the latest connection identifier, but they send CloseSocket while we have outgoing connections, then they've tried to close the socket too early. There's a test case for this (testEarlyCloseSocket) demanding that EventConnectionLost is posted in this case.

avieth commented 7 years ago

@edsko @dcoutts

dcoutts commented 7 years ago

This looks good to me. I think we should also rename remoteMaxIncoming to remoteLastIncoming, but the code for updating it is already correct (it was not doing any max calc, just relying on increasing sequences).

avieth commented 7 years ago

This looks good to me. I think we should also rename remoteMaxIncoming to remoteLastIncoming, but the code for updating it is already correct (it was not doing any max calc, just relying on increasing sequences).

Done

dcoutts commented 7 years ago

I think this is good to merge. @qnikst any objections?

qnikst commented 7 years ago

everything ok from my side.