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

Conflicts getting ahead of queued pushes? #804

Open
michielbdejong opened this issue Oct 16, 2014 · 10 comments
Open

Conflicts getting ahead of queued pushes? #804

michielbdejong opened this issue Oct 16, 2014 · 10 comments
Labels

Comments

@michielbdejong
Copy link
Member

this starter-kit bug looks like a problem with the library. It is easy to replicate, @silverbucket and I have both seen it happen.

It looks like maybe there are conflicts coming in while outgoing pushes are still being queued. Maybe they're queued in https://github.com/remotestorage/remotestorage.js/blob/master/src/cachinglayer.js#L306-L315, maybe in the commit-cache, maybe just in sync tasks. Does make me think that maybe we have too many queues! ;) Should nonetheless be easy to fix, if you just reproduce it and trace what happens.

@silverbucket
Copy link
Member

Sounds complicated :) I don't know enoug of this area of the code (and don't have a lot of expereince with syncing concepts in general) to be able to help out at the moment... but I whole-heartedly agree that this area of the code is overly complex / too many code paths.

@raucao raucao removed the ready label Nov 7, 2014
@michielbdejong
Copy link
Member Author

@silverbucket You added the ready label but then @skddc removed it... This is quite serious though, if it even makes the starter-kit hello world example fail. We should probably find time to fix this. Do you want to pair on this?

@raucao
Copy link
Member

raucao commented Mar 13, 2015

I removed it, because we don't use that label. It was automatically added by a tool, that only @silverbucket uses personally.

@silverbucket
Copy link
Member

yeah, sorry about that - i've since disabled that feature (was doing it automatically)

@karlb
Copy link

karlb commented Dec 14, 2023

Any updates on this? I think I'm still experiencing this problem with v2.0.0-beta.6 (but also earlier versions).

@raucao
Copy link
Member

raucao commented Dec 14, 2023

Wow, this is one is ancient! 😳

@karlb Could you provide an example to reproduce it perhaps? The starter kit that the bug has initially been created for has been deprecated for a very long time.

@karlb
Copy link

karlb commented Dec 15, 2023

I don't have a minimal example, but when I type quickly at https://static.karl.berlin/doagain/ while being signed in to my 5apps remote storage, I get a conflict despite my browser being the only device sending writes. This sounded similar to this issue, but without further research it is just a guess.

@raucao
Copy link
Member

raucao commented Dec 15, 2023

Oof, that app is doing immediate writes to the same document on every keystroke with zero debouncing. That's definitely never a good idea, aside from potential race conditions in the library!

However, I think the bug's explanation is that outgoing pushes/PUTs to the server are queued up for successive sync immediately. And when there's a push still outgoing that hasn't updated the current revision (ETAG) yet, then the already queued next push would use the outdated ETAG and thus fail. (Just recapping the issue to be solved).

@karlb
Copy link

karlb commented Dec 16, 2023

Oof, that app is doing immediate writes to the same document on every keystroke with zero debouncing. That's definitely never a good idea, aside from potential race conditions in the library!

You are totally right of course and I also noticed while looking at the problem. I just wanted to understand the conflict problem before, so that I don't just hide the problem. Now that I improved my conflict handling and the bug is at least tracked properly, I'll go and add the debouncing.

Thanks for looking into it and your (and everyone else's) work remotestorage!

karlb added a commit to karlb/doagain that referenced this issue Dec 16, 2023
We don't want to spam the remoteStorage server with save requests. It
also avoids triggering
remotestorage/remotestorage.js#804 .
@DougReeder
Copy link
Contributor

As debouncing is required, that ought to be the responsibility of the library, rather than every single app that uses it. I'd be happy to work on that once this issue is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants