Package Syncing

[kpratyus]

Making sure that only one package syncing operation happens at a time. #84

[codecov]

Codecov Report

Merging #118 (6a15fe0) into main (3d4cbdc) will increase coverage by 1.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   74.89%   76.22%   +1.33%     
==========================================
  Files          22       22              
  Lines        1629     1632       +3     
==========================================
+ Hits         1220     1244      +24     
+ Misses        307      292      -15     
+ Partials      102       96       -6     

Impacted Files	Coverage Δ
client/internal/clientstate.go	90.47% <ø> (ø)
client/internal/packagessyncer.go	59.81% <100.00%> (+0.55%)	⬆️
client/internal/receivedprocessor.go	84.28% <0.00%> (+15.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d4cbdc...6a15fe0. Read the comment docs.

[tigrannajaryan]

@kpratyus Thanks for the PR!
Is it possible to add a test that tries to race (e.g. quickly return 2 successive PackagesAvailable offers from the Server to trigger 2 concurrent sync operations) and demonstrates that this fix works correctly?

[tigrannajaryan]

I don't think this is the right place for this mutex. The ClientSyncedState struct has a specific purpose that is described above and adding a mutex for packager syncer does not fit that purpose.

receivedProcessor may be a better place for this mutex.

[kpratyus]

The receivedProcessor is constructed in the constructor of wsReceiver. As the mutex has to be unique for each client and the pointer has to be passed to `receivedProcessor'. So mutex has to be included in the constructor of client.common, wsReceiver and receivedProcessor. To avoid these many changes I added the mutex in ClientSyncedState. What's your opinion on this?

[tigrannajaryan]

There is one-to-one relationship between Client, ClientSyncedState and receivedProcessor, so I don't understand where do you see a problem. A mutex declared in receivedProcessor struct will be unique for each client, just like it is unique when it is in ClientSyncedState. There is no need to pass the mutex to receivedProcessor, receivedProcessor can contain the mutex.

[kpratyus]

Correct me if I am wrong here. For every client The function runOneCycle runs in a loop. Everytime the function runs it creates a new receiver object.
r := internal.NewWSReceiver( c.common.Logger, c.common.Callbacks, c.conn, c.sender, &c.common.ClientSyncedState, c.common.PackagesStateProvider, c.common.Capabilities, )
This in turn creates a new receivedProcessor' object everytime. processor: newReceivedProcessor(logger, callbacks, sender, clientSyncedState, packagesStateProvider, capabilities)`
So how is there a one to one relationship between client and receivedProcessor?

[tigrannajaryan]

You are absolutely right and I was wrong! Due to reconnections receivedProcessor may be re-created while a previous sync is still happening. So, receivedProcessor is the wrong owner for the mutex, you are right.

I still don't like putting it in ClientSyncedState, it doesn't fit the purpose.

Here are a couple options:

• Extend the purpose of ClientSyncedState to contain all of the client state. In that case the mutex can be a good fit there. It probably needs to be renamed to ClientState. I am not entirely sure this is a good idea though, since we are not really putting the entire client state in it, so it may become misleading.
• Another good owner for the mutex is ClientCommon. However, this will require passing the mutex where receivedProcessor is created and store the pointer to the mutex in receivedProcessor. Probably a bit awkward but doable.

Maybe there is another option that I haven't thought of, please feel free to suggest.

[kpratyus]

While raising the PR I also thought of these 2 options. Code clarity wise adding all the mutexes in a single structure(ClientSyncedState here) makes sense but I totally get the point that we want to keep ClientSyncedState as lightweight as possible.

Although awkward I think keeping it in ClientCommon is a better option. In future if there arises need of any other locks then it would be totally extendable as well.

[tigrannajaryan]

Although awkward I think keeping it in ClientCommon is a better option. In future if there arises need of any other locks then it would be totally extendable as well.

Sounds good, let's go with this approach.

[tigrannajaryan]

Why isn't the mutex locked right from the top of doSync? AllPackagesHash is also susceptible to the race. It reads data that is written by SetAllPackagesHash below.

[kpratyus]

Ah, Missed that. I actually thought that it only handles logging. Will change the mutex. Thanks for pointing it out

[kpratyus]

@kpratyus Thanks for the PR!
Is it possible to add a test that tries to race (e.g. quickly return 2 successive PackagesAvailable offers from the Server to trigger 2 concurrent sync operations) and demonstrates that this fix works correctly?

I will try to add this test