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

Review concepts of "document has changed", avoid race conditions #51

Open
gterzian opened this issue Sep 15, 2023 · 3 comments
Open

Review concepts of "document has changed", avoid race conditions #51

gterzian opened this issue Sep 15, 2023 · 3 comments

Comments

@gterzian
Copy link
Collaborator

gterzian commented Sep 15, 2023

In the light of the dicussion at #21 (comment)

We may want to review the use of the last_heads concept inside note_changes(see current WIP), because it can lead to races conditions between saving and sending out sync messages or waking up change observers(see process_changed_documents.

@issackelly
Copy link
Contributor

A note on separating out the types of reads and writes:

One of the major reasons that we're migrating to incremental saves + BEC syncing is to fix deadlock conditions in our stack. I think it would be safer to do a try_read() lock and try_write() type locks and fail with errors. In situations where it can be delayed, deferred or recoverable, we should do that.

@gterzian
Copy link
Collaborator Author

gterzian commented Sep 18, 2023

I agree about the need to prevent deadlocks.

In this particular issue, the problem was rather a livelock, in the sense that something that was expected to happen(sending out sync messages), did not(and it was not caused by an underlying deadlock). So it was rather the logic of the code that was wrong and produced a livelock.

I don't think we should switch to using try_ methods on the lock acquisitions inside repo.rs, because I don't think those can produce deadlocks(minimal scoping, no other locks being acquired in scope). Switching to "trying to acquire locks and perform certain operations" would also imply adding logic to "retry" if the lock cannot be acquired, which would add complication.

There is one potential source of deadlock I have just noted on the doc handle side of things, and it's tracked at #53.

@issackelly
Copy link
Contributor

Thank you! I appreciate the context

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

No branches or pull requests

2 participants