Skip to content

[local storage] Remove IPC lock file after releasing the lock - #2172

Open
Sanjays2402 wants to merge 1 commit into
apache:trunkfrom
Sanjays2402:fix/local-lock-file-cleanup
Open

[local storage] Remove IPC lock file after releasing the lock#2172
Sanjays2402 wants to merge 1 commit into
apache:trunkfrom
Sanjays2402:fix/local-lock-file-cleanup

Conversation

@Sanjays2402

Copy link
Copy Markdown
Contributor

Closes #1975

Description

fasteners.InterProcessLock creates its lock file but never removes it, and
LockLocalStorage.__exit__ did not remove it either. The file name is a sha256
of the locked path, so one file was left behind in the temp directory for every
distinct path ever locked -- a long running process writing many objects can
exhaust inodes. The lock file is now removed after the IPC lock is released.

Status

done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

fasteners.InterProcessLock creates its lock file but never removes it, and
LockLocalStorage did not remove it either. Because the file name is derived
from a sha256 of the locked path, one file was left behind in the temporary
directory for every distinct path ever locked, which can exhaust inodes on
long running processes that write many objects.

The lock file is now removed after the IPC lock is released.

Closes apache#1975

@micafer micafer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing the lock file immediately after releasing InterProcessLock introduces a race which can break mutual exclusion.

For example, process B may acquire the existing file lock immediately after process A releases it. A can then unlink the path, and process C can create and lock a new file at the same path. B and C would consequently hold locks on different inodes and could both enter the critical section.

The new test only verifies that the file is removed in a non-concurrent scenario, so it does not cover this race or verify that mutual exclusion is preserved.

I don't think the lock file can safely be removed as part of exit. We likely need a different bounded-lock or out-of-band cleanup strategy.

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.

Improper lock usage leads to inode overflow in LockLocalStorage implementation

2 participants