trachten / cpisync

A library for synchronizing remote data with minimum communication.
GNU General Public License v3.0
26 stars 11 forks source link

SyncTest socket reuse bug #48

Closed novakboskov closed 4 years ago

novakboskov commented 4 years ago

SyncTest function when used with GenSync over CommSocket forks two pairs of GenSyncClient, GenSyncServer. Both pairs reconcile over the same socket. Since one of the pairs must finish first, the one behind fails to close the socket. It's already closed by that time.

I tried to depict the scenario (the socket is in blue):

SyncTest_issue

The bug can easily be reproduced by running IBLTSyncTest or CuckooSyncTest with DEFAULT_LOGLEVEL=METHOD.

That prints the message Attempted closing of socket that is not connected to anything. from CommSocket::commClose().

The first pair of GenSyncClient, GenSyncServer closes the socket when done and sets CommSocket::my_fd to -1, the trailing pair of GenSyncClient, GenSyncServer fails to close the socket because CommSocket::my_fd == -1 by that time.

arorashu commented 4 years ago

Posting an update.

So, I found that this issue has been somewhat discussed in the BUGS.txt, and also in the code

arorashu commented 4 years ago

PR #61 is created for this issue. The architecture of syncTest is as follows:

cpiSync syncTest

arorashu commented 4 years ago

Actually, I have further simplified it now: simplify-cpisync-test

arorashu commented 4 years ago

I have written here semi-formally how syncTest is expected to behave:

For all syncs that implement GenSync, syncTest tests that the following guarantees hold:

Let Peer P1 be client with data - set A : GenSyncClient and Peer P2 be server with data - set B : GenSyncServer

Now, GenSyncClient synchroizes using GenSync :: clientSyncBegin, and GenSyncServer synchroizes using GenSync :: serverSyncBegin

At the end, GenSyncServer data is modified. The server is now expected to have the data - set A U B. For "one-way" syncs, the client should be unchanged, else the client is also modified to contain data - A U B

If the resultant data set for GenSyncClient and GenSyncServer is as expected, the test passes, else the test fails.

trachten commented 4 years ago

Looks right to me!

On Jun 19, 2020, at 8:34 AM, 6/19/20, Shubham Arora notifications@github.com wrote:

I have written here semi-formally how syncTest is expected to behave:

For all syncs that implement GenSync, syncTest tests that the following guarantees hold:

Let Peer P1 be client with data - set A : GenSyncClient and Peer P2 be server with data - set B : GenSyncServer

Now, GenSyncClient synchroizes using GenSync :: clientSyncBegin, and GenSyncServer synchroizes using GenSync :: serverSyncBegin

At the end, GenSyncServer data is modified. The server is now expected to have the data - set A U B. For "one-way" syncs, the client should be unchanged, else the client is also modified to contain data - A U B

If the resultant data set for GenSyncClient and GenSyncServer is as expected, the test passes, else the test fails.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/trachten/cpisync/issues/48#issuecomment-646611189, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACM5LU46ZI6XSFJJIILRILLRXNLMHANCNFSM4MFDJLHQ.


Ari Trachtenberg, Boston University http://people.bu.edu/trachten mailto:trachten@bu.edu

Random quote of the day: 
A day without sunshine is like, night. 
arorashu commented 4 years ago

This issue was fixed in PR#61. Closing here.