Modernize and cleanup locking #72
Conversation
- Move from try/finally to with - Start phasing out the _lock_acquire/_lock_release shortcuts - Replace simpler @locked decorators with with statements. I've come to prefer the with style tp the decorator style. I think it's clearer. (The decorators preceeded the Python with statement.) I left the decorators in cases where they were used with preconditions. This is mostly pretty mechanical, although it got a little delicate in places and ... tests
| @@ -85,13 +85,13 @@ def __init__(self, name, base=None): | |||
|
|
|||
| # Allocate locks: | |||
| self._lock = utils.RLock() | |||
| self.__commit_lock = utils.Lock() | |||
| @@ -445,11 +442,10 @@ def close_files_remove(): | |||
| # pack didn't free any data. there's no point in continuing. | |||
| close_files_remove() | |||
| return None | |||
| self._commit_lock_acquire() | |||
| self._commit_lock.acquire() | |||
There was a problem hiding this comment.
Since acquire/release all occur in the same method, wouldn't this be clearer using with self._commit_lock:?
There was a problem hiding this comment.
OK, I see that copyRest -> copyOne temporarily releases the lock. This smells a bit.
There was a problem hiding this comment.
This packing code is very old and really ought to be updated. But this is out of scope of this PR.
|
@tseaver are you good with this now? |
| self._commit_lock_acquire = self.__commit_lock.acquire | ||
| self._commit_lock_release = self.__commit_lock.release | ||
| self._commit_lock_acquire = self._commit_lock.acquire | ||
| self._commit_lock_release = self._commit_lock.release |
There was a problem hiding this comment.
Are these micro-optimizations still worth preserving?
There was a problem hiding this comment.
They're part of the old inheritance API. :( The comment above explains this.
There was a problem hiding this comment.
Ugh, what a terrible pattern: who could have done such a thing? (he asks innocently).
|
OK to merge even without addressing my release-then-acquire concerns (it still smells like a bug magnet, though). |
|
I thought I did address your concerns. Did you see the change I made? The locked regions for the ordinary lock should be very obvious now. |
|
The one that I commented on last me the |
|
OK, wrt commit lock being held over multiple methods: a) This is a key aspect of the two-phase commit protocol. At least when we return from voting, we have to be assured that we can commit the modified objects, so we have to prevent modification while in the second phase. Currently, we get the lock in tpc_begin. This is a design flaw. We don't really need to get the vote until tpc_vote, and of course, we should be able to get object-level locks, rather than a global lock. This is related to #69. So, eventually we won't get locks until tpc_vote, but even then, the lock will be acquired in tpc_vote and released in tpc_finish or tpc_abort. b) This is out of scope of this change. :) Although, I think using |
Move from try/finally to with
Start phasing out the _lock_acquire/_lock_release shortcuts
Replace simpler @locked decorators with with statements.
I've come to prefer the with style tp the decorator style.
I think it's clearer.
(The decorators preceeded the Python with statement.)
I left the decorators in cases where they were used with preconditions.
This is mostly pretty mechanical, although it got a little delicate in
places and ... tests