fix(sync): prevent race condition by relying on autoincrement #4938
Conversation
max-nextcloud
force-pushed
the
fix/4600-prevent-race-condition
branch
2 times, most recently
from
9182a19
to
e9dcefc
Compare
|
Obviously WIP - opened the PR to be able to discuss the approach. |
mejo-
force-pushed
the
fix/4600-prevent-race-condition
branch
2 times, most recently
from
1503b54
to
a5175cf
Compare
|
I like the approach, it's a way cleaner solution than my approach to make read+write of |
|
Successfully tested the following scenario:
All change are synced between all clients as expected. |
|
Found some weirdness when giving it a quick test, but not sure if related to this PR or not:
Edit: Also happens on main it seems |
|
There are two things that i did not look into yet that might be worthwhile ensuring:
|
Prevent a possible race condition when two clients add steps at the same time. See #4600. Rely on the autoincrementing id in order to provide a canonical order that steps can be retrieved in. When two clients push steps at the same time the entries receive destinct ids that increment. So if another client fetches steps in between it will see the smaller id as the version of the fetched step and fetch the other step later on. Transition: In the future we can drop the version column entirely but currently there are still steps stored in the database that make use of the old column. So we need to transition away from that. In order to find entries that are newer than version x we select those that have both a version and an id larger than x. Entries of the new format are newer than any entry of the old format. So we set their version to the largest possible value. This way they will always fulfill the version condition and the condition on the id is more strict and therefore effective. For the old format the version will be smaller than the id as it's incremented per document while the id is unique accross documents. Therefore the version condition is the more strict one and effective. The only scenario where the version might be larger than the id would be if there's very few documents in the database and they have had a lot of steps stored in single database entries. Signed-off-by: Max <[email protected]> Signed-off-by: Jonas <[email protected]>
The value used before (largest possible MySQL BIGINT value) was too big for PHP int. Since we still support 32-bit platforms on Nextcloud, let's stick to the largest possible 32-bit PHP integer value. Besides, setting the value as default for `Step::version` doesn't work as `QBMapper->insert()` doesn't recognize the `version` field as changed in that case. So let's default to `0` again and set it using `Step->setVersion()` later. Signed-off-by: Jonas <[email protected]>
juliusknorr
force-pushed
the
fix/4600-prevent-race-condition
branch
from
a5175cf
to
308d640
Compare
I thought a bit about this and I think we should ignore this corner case. Nextcloud instances with just one (or very few) document sessions likely aren't used collaboratively. Sure, there's a theoretical possibility that people setup a Nextcloud instance to work together on one file, but that's not very likely. And even in this scenario, the autosave should have saved the last document state. So worst case in this very unlikely scenario people will loose a few editing steps. This is not worth adding complex migration logic in my opinion. |
@juliushaertl and me looked into this. The index on |
|
/backport ea208ee,308d64069f8903a113985d7d71d027b127a3b0ce to stable27 |
Prevent a possible race condition when two clients add steps at the same time.
See #4600.
Rely on the autoincrementing id in order to provide a canonical order that steps can be retrieved in.
When two clients push steps at the same time
the entries receive destinct ids that increment.
So if another client fetches steps in between
it will see the smaller id as the version of the fetched step and fetch the other step later on.
Transition:
In the future we can drop the version column entirely but currently there are still steps stored in the database that make use of the old column.
So we need to transition away from that.
In order to find entries that are newer than version x we select those that have both a version and an id larger than x.
Entries of the new format are newer than any entry of the old format. So we set their version to the largest possible value. This way they will always fulfill the version condition and the condition on the id is more strict and therefore effective.
For the old format the version will be smaller than the id as it's incremented per document while the id is unique accross documents. Therefore the version condition is the more strict one and effective.
The only scenario where the version might be larger than the id would be if there's very few documents in the database and they have had a lot of steps stored in single database entries.
📝 Summary
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)