Sanjays2402 commented on PR #2172: URL: https://github.com/apache/libcloud/pull/2172#issuecomment-5206742063
You're right, and the race is worse than the ordering I had in mind. Even if I move the `os.remove` to before `release()` so the unlink happens while the lock is still held, it doesn't help: a process already blocked in `acquire()` is blocked on an fd for the *old* inode, so it wakes up holding a lock on an unlinked file while the next process creates a fresh file at the same path and locks that. Two holders, different inodes, exactly the scenario you described. There is no ordering of unlink and release inside `__exit__` that avoids it, because the identity of the lock is the inode and the lookup key is the path. The only ways I can see to make it safe are all heavier than the leak deserves: - a second, permanent lock file guarding create+unlink of the real one, which just moves the leak to the guard file (one per path, same cardinality); - refcounting holders in the file itself, which needs its own atomicity story across processes; - out-of-band cleanup (age-based sweep of `<tmp>/lock/` at driver init or a maintenance entry point), which is safe but is a different feature from what this PR claims. And the thing being leaked is a zero-byte file per distinct locked path, bounded by the set of paths the process touches, in a directory the OS already reclaims. That's a poor trade against any risk to mutual exclusion. So I'll concede this one — the approach in the PR is not fixable in place. Happy to close it. If you think the age-based sweep is worth having as a separate change I can open that instead, but I'd rather you say so than have another speculative PR land in your queue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
