Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable sync lock support #10407

Closed
laurent22 opened this issue May 7, 2024 · 4 comments
Closed

Disable sync lock support #10407

laurent22 opened this issue May 7, 2024 · 4 comments
Labels
desktop All desktop platforms enhancement Feature requests and code enhancements high High priority issues mobile All mobile platforms

Comments

@laurent22
Copy link
Owner

laurent22 commented May 7, 2024

Add an option, under Feature Flags, to disable sync locks. I think they are no longer necessary since they exist for sync target upgrades, but we don't do this anymore.

Joplin sync itself has never required sync locks since all operations are atomic.

First we put the option in the feature flags and once we are confident it works well without locks we make it the default.

@laurent22 laurent22 added enhancement Feature requests and code enhancements mobile All mobile platforms desktop All desktop platforms high High priority issues labels May 7, 2024
@roman-r-m
Copy link
Collaborator

all operations are atomic

what about this scenario?

  1. client A reads a note from the server, sees it hasn't been updated
  2. before it can update the note, client B pushes its changes
  3. client A pushes its change overwriting B's update

but then, i don't think a sync lock would've prevented it anyway

@laurent22
Copy link
Owner Author

You're right, I don't think we support this. The individual write/delete operations are atomic but checking some info, then writing something based on it is not. I guess it's never been a problem since it would require changes to be made almost at the exact same time on two different clients. And indeed the sync locks would not help with this.

@roman-r-m
Copy link
Collaborator

tbh, i think sync should just lock the whole thing like the exclusive lock does, it'd make reasoning about sync much easier
yes it'd prevent multiple clients from syncing at the same time so what

@laurent22
Copy link
Owner Author

Reasoning about sync, maybe, but it also brings extra complexity especially when things don't go as expected. We need to ensure sync locks can expire, but we also need to make sure they don't expire too quickly so that someone with a slow connection doesn't get locked out. Also if you import a huge ENEX file for example and sync it on desktop, meanwhile you can't use your phone at all to sync your notes.

There are probably other potential issues we aren't familiar with (since we never tried to make sync lock exclusive) and it's possible we'd simplify thing on one side but make it more complex on another side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms enhancement Feature requests and code enhancements high High priority issues mobile All mobile platforms
Projects
None yet
Development

No branches or pull requests

2 participants