refactor(locks): delegate cross-platform file locking to portalocker - #100
Merged
Conversation
Replace the hand-rolled fcntl/msvcrt flock/funlock (merged in #99) with portalocker, which is fcntl-backed on POSIX and msvcrt/Win32-backed on Windows with maintained, cross-platform-tested behaviour. Removes the hand-written Windows retry/timeout loop that could not be exercised on POSIX. - flock/funlock now delegate to portalocker.lock/unlock. - Drop the guarded 'import fcntl', the msvcrt fallback, and _WINDOWS_LOCK_TIMEOUT. - Keep the os.fchmod guard and Windows directory-fsync skip (atomic writes, which portalocker does not cover). - Pin portalocker==3.2.0 (BSD-3) to match the exact-pin dependency policy. Note: true shared (reader) locks on Windows would still need pywin32; without it portalocker uses msvcrt (exclusive). Not added — in-process concurrent KB reads are rare. Refs #93. Tests: swap the msvcrt-internals tests for a cross-process exclusion test that verifies flock takes a real OS lock, plus the retained atomic-write/fsync-skip guards. Full suite 749 passed.
![github-code-quality[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…rdening Addresses /code-review findings on the portalocker refactor: - flock docstring corrected: portalocker uses Win32 LockFileEx (pywin32, pulled in automatically on Windows) for SHARED locks, so concurrent readers ARE honoured; EXCLUSIVE uses msvcrt (retries ~10s then raises, not an infinite block); failures raise portalocker.LockException, not OSError. - Re-add the issue #93 regression guard: assert no openkb module hard-imports the Unix-only fcntl at module level (replaces the dropped import-without-fcntl test without depending on portalocker internals). - Strengthen the cross-process lock test: assert both BLOCKED (while held) and ACQUIRED (after release), and check the probe's exit code so an ImportError surfaces clearly instead of an empty-stdout false failure. - Drop the now-dead 'import pytest' / 'import portalocker' from the test module.
Drop the unused 'import openkb' and derive the package dir from locks.__file__ instead, so the test module uses one import style for openkb (github-code-quality bot). The unused 'import portalocker' was already removed in the prior commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the hand-rolled
fcntl/msvcrtflock/funlockadded in #99 withportalocker— fcntl-backed on POSIX, msvcrt/Win32-backed on Windows, with maintained, cross-platform-tested behaviour.Motivation: the #99 Windows fallback was hand-written
msvcrtlogic (retry loop, timeout, byte-range locking) that could not be exercised on POSIX, so its Windows behaviour was unverified. Delegating to a vetted library moves the untestable platform path to code its maintainers test on Windows.Changes
flock/funlocknow callportalocker.lock/portalocker.unlock.import fcntl, themsvcrtfallback, and_WINDOWS_LOCK_TIMEOUT/ retry loop.os.fchmodguard and the Windows directory-fsyncskip (atomic writes — not covered by portalocker).portalocker==3.2.0(BSD-3) to match the exact-pin dependency policy.portalockerdeclarespywin32undersys_platform == 'win32', so it is installed automatically on Windows (no transitive dep on POSIX).Windows behaviour (verified against portalocker 3.2.0 source)
LockFileExAPI (via the auto-installedpywin32) for shared locks, so concurrent readers work. (Correcting an earlier draft of this PR that wrongly said shared locks need a manual pywin32 add.)msvcrt.locking, which retries ~10s then raises rather than blocking indefinitely. A concurrent exclusive waiter contending with a long-held lock fails fast (LockException) instead of queueing — acceptable for OpenKB's usage; documented in theflockdocstring.portalocker.LockException(notOSError).Testing
portalocker.lockisBLOCKEDwhile the parent holds the lock andACQUIREDafter release (both directions), asserting the probe's exit code so an env/import error surfaces clearly.openkbmodule hard-imports the Unix-onlyfcntlat module level (so a future regression re-breaking Windows import is caught).tests/test_locks.py(feat: add KB mutation locks and atomic state writes #86kb_lockbehaviour) still passes — delegation preserves the lock semantics.Supersedes the hand-rolled approach in #99. Refs #93.