Skip to content

SQLite-based read-write lock #399

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

SQLite-based read-write lock #399

wants to merge 7 commits into from

Conversation

leventov
Copy link

@leventov leventov commented Feb 19, 2025

Draft for #307.

@gaborbernat @Yard1 please let me know how I should change the code from the API perspective. There are quite a bit very elaborate stuff that is hard to understand regarding thread-locals, etc.

@leventov leventov marked this pull request as draft February 19, 2025 18:24
super().__init__(lock_file, timeout, blocking)
self.procLock = threading.Lock()
self.con = sqlite3.connect(self._context.lock_file, check_same_thread=False)
# Redundant unless there are "rogue" processes that open the db
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align comments to 120 charachters 🙇



class _SQLiteLock(BaseFileLock):
def __init__(self, lock_file: str | os.PathLike[str], timeout: float = -1, blocking: bool = True) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer

from os import PathLike

@leventov
Copy link
Author

@gaborbernat I've pushed a more "serious" attempt.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, can you add tests please?

@leventov
Copy link
Author

@gaborbernat what do you think about using SQLite's VFS interface directly, without the "dummy database" thing? https://github.com/rogerbinns/apsw provides bindings

@gaborbernat
Copy link
Member

gaborbernat commented Feb 25, 2025 via email

@leventov
Copy link
Author

leventov commented Mar 4, 2025

@gaborbernat I've added tests.

If you want to finesse API (context managers vs. parameters to acquire_read() and acquire_write() methods; are read_lock() and write_lock() methods even needed? Are instance's default timeout and blocking needed?), docs, formatting, class/method naming, etc., could you please do it yourself to your liking?

@leventov leventov marked this pull request as ready for review March 8, 2025 10:59
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failing.

@leventov
Copy link
Author

@gaborbernat I don't need this PR to be merged to use it later through filelock library. In fact, I submitted a PR only to attract code review and discussion. Myself, I'm going to use this code copied (and modified to an extent) in the codebase. So, I'm not invested in the merge per se.

What of these seem valid to you, and what seem like false positives?

src/filelock/_read_write.py:61: error: Returning Any from function declared to return "ReadWriteLock"  [no-any-return]
src/filelock/_read_write.py:61: error: "_ReadWriteLockMeta" has no attribute "get_lock"  [attr-defined]
src/filelock/_read_write.py:62: error: Returning Any from function declared to return "ReadWriteLock"  [no-any-return]
src/filelock/_read_write.py:67: error: Need type annotation for "_instances"  [var-annotated]
src/filelock/_read_write.py:77: error: Argument 2 for "super" not an instance of argument 1  [misc]
src/filelock/_read_write.py:91: error: Returning Any from function declared to return "ReadWriteLock"  [no-any-return]

Can the CI auto-fix from "ReadWriteLock" to ReadWriteLock itself cause some of these errors?

check / test pypy3.10 - windows-latest is test-suite related, perhaps even CI environment-related.

@gaborbernat gaborbernat marked this pull request as draft March 17, 2025 15:51
@gaborbernat
Copy link
Member

Moved to draft until we fix the issues.

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.

2 participants