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

Make read and write locks useful #15

Merged
merged 5 commits into from Jan 2, 2023
Merged

Make read and write locks useful #15

merged 5 commits into from Jan 2, 2023

Conversation

plainerman
Copy link
Contributor

This PR would close #14. It servers as a proposal to discuss the direction we should go to solve this problem. If this is the way we want to go, I can extend it to the remaining useful functions and we can merge the PR.

@plainerman plainerman mentioned this pull request Dec 18, 2022
@ex0dus-0x
Copy link
Owner

Thanks for bringing up your concerns and starting the PR! Your suggestions make sense, as locked operations certainly would require some manual sanitization/encryption on the users end. This was not prior since it wasn't something I needed for my use case (I only cared about basic primitives most of the time), but would be nice for other's.

PR looks good to me so far, be sure to lint and add test cases too :)

@plainerman
Copy link
Contributor Author

Great, then I will do the same for the remaining functions, document the changes, and see that the pipeline passes.

One question still:
I would say, that after write_lock the database should auto_commit (if the variable is set). Do you agree?

@ex0dus-0x
Copy link
Owner

That sounds good! It looks like auto_commit doesn't have much of an effect at the moment, so would definitely appreciate adding the functionality there.

@plainerman plainerman marked this pull request as ready for review December 30, 2022 18:27
@plainerman
Copy link
Contributor Author

Could you look at the changes once more?
I moved the commit part into the lock function, added doc-tests, and simplified the calls from the namespace class.

From my side, the changes are good to go, since the previous tests also test the new code.

This does not hurt but allows moving variables from the closure
@ex0dus-0x ex0dus-0x merged commit 3f7187b into ex0dus-0x:master Jan 2, 2023
@ex0dus-0x
Copy link
Owner

Thanks @plainerman for your contributions! The refactoring to lock acquisition and passing in a callback all make sense to me, and am also grateful for getting the auto_commit functionality to actually operate.

Appreciate all the help with this project!

@plainerman
Copy link
Contributor Author

Thank you very much for merging this so quickly!

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 this pull request may close these issues.

Usefulness of locks
2 participants