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

Add validator to local writes that happen during a bootstrap #6098

Closed

Conversation

danieltabacaru
Copy link
Collaborator

@danieltabacaru danieltabacaru commented Dec 9, 2022

What, How & Why?

History can diverge in flexible sync if writes occur during bootstraps. Only writes to objects that come into/go out of view are affected, so we disallow it. This happens because each side (client & server) sees their write as the most recent one. WriteNotAllowed will be thrown in such cases.
Fixes #5804.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Dec 9, 2022
@danieltabacaru
Copy link
Collaborator Author

Tests will be added, but there is no reason to hold back on reviewing the changes.

@danieltabacaru danieltabacaru marked this pull request as ready for review December 9, 2022 12:34
@tgoyne
Copy link
Member

tgoyne commented Dec 9, 2022

I don't think this use of Mixed is safe. For string primary keys, the data it is pointing at may no longer be valid when we actually read the value, as it's being stored over the duration of multiple write transactions.

This also has API implications. The functional result of this change in Swift is that attempting to write to an object mid-bootstrap will result in the app crashing, and there's no way for the developer to check if the object is a legal one for them to write to first. Even if they could, that'd still add up to a breaking change; it implies that every app which uses flexible sync and modifies existing objects would need to check that they can legally modify an object before each change.

@danieltabacaru
Copy link
Collaborator Author

I don't think this use of Mixed is safe. For string primary keys, the data it is pointing at may no longer be valid when we actually read the value, as it's being stored over the duration of multiple write transactions.

This also has API implications. The functional result of this change in Swift is that attempting to write to an object mid-bootstrap will result in the app crashing, and there's no way for the developer to check if the object is a legal one for them to write to first. Even if they could, that'd still add up to a breaking change; it implies that every app which uses flexible sync and modifies existing objects would need to check that they can legally modify an object before each change.

We discussed several solutions and the reason we landed on this one is its simplicity and the fact that this is more of an edge case. I would expect that most users will wait for bootstrap to end before doing anything with the data.

I am not sure I understand what the problem with string primary keys is.

@tgoyne
Copy link
Member

tgoyne commented Dec 9, 2022

Blocking write transactions entirely until the bootstrap completes seems like a much more reasonable solution than crashing if the user tries to write to an object during that time.

Mixed does not own the buffer that it points to when it stores a StringData, and instead it's a pointer into the Realm file. Because that pointer is held over the course of multiple transactions, the data which it is pointing to can be overwritten by a subsequent write and no longer be valid when it's used for validation.

@danieltabacaru
Copy link
Collaborator Author

danieltabacaru commented Dec 10, 2022

We can solve the Mixed problem by storing the string representation instead.

We throw in a similar validation case when writing to a table without a subscription.

We could block the write transactions, but we need to release the write lock so the bootstrap can proceed. So I guess we would need to turn it into a read transaction first, block, and then promote it to write when bootstrap is done. At the same time this would defeat the purpose of a project we did to release the write lock so the user can commit.

@tgoyne
Copy link
Member

tgoyne commented Dec 10, 2022

Writing to a table without a subscription is a precondition that the user can check, and in most cases would only happen due to user error. If a user is getting crashes from that, there's some pretty straightforward answers we can give them about how to avoid it.

That's not the case here. If a user asks how to avoid this crash, the only answer we can give them is to avoid performing writes if any subscriptions are in the middle of applying bootstrapping. If that's the intended answer, then we really should just be applying the bootstraps in a single write transaction.

@danieltabacaru
Copy link
Collaborator Author

If that's the intended answer, then we really should just be applying the bootstraps in a single write transaction.

I don't think we want to go this route because there may be a lot of data. We've seen problems with mmap because of that.

@danieltabacaru
Copy link
Collaborator Author

Since this may be too confusing to users, we will apply the bootstrap messages continuously, not allowing the user to commit changes until bootstrap is done.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History can diverge in FLX sync if writes occur during bootstraps
2 participants