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

Fix MagicFolderConfig.store_local_snapshot to use transactions correctly. #382

Closed
tomprince opened this issue Jun 3, 2021 · 1 comment · Fixed by #470
Closed

Fix MagicFolderConfig.store_local_snapshot to use transactions correctly. #382

tomprince opened this issue Jun 3, 2021 · 1 comment · Fixed by #470

Comments

@tomprince
Copy link
Contributor

It uses the with_cursor decorator, but calls it self recursively. If I am reading the sqlite docs and sqlite module docs correctly, this won't do the right thing, as the transaction type used is not recursive.

It looks like this is the only function that runs into this issue, but it would be good to make with_cursor resilient to this kind of error as well.

@tomprince
Copy link
Contributor Author

It turns out that it does use transaction incorrectly, as the recursive call will cause the transaction in the parent call to be committed, without having the parent links written. If we crash immediately after that commit, we won't be able to load snapshots for that file, since there will be two leaf snapshots, and we will hit this exception.

There is also a transaction issue in MagicFolderConfig.get_local_snapshot accessing the .author property, which is itself decorated with with_cursor. However, this case is less of an issue since they are both read-only transactions.

@tomprince tomprince changed the title Verify that MagicFolderConfig.store_local_snapshot uses transactions correctly. Fix MagicFolderConfig.store_local_snapshot to use transactions correctly. Jul 1, 2021
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

Successfully merging a pull request may close this issue.

1 participant