Wait on locks #15
Wait on locks #15
Conversation
pytest_lab/locker.py
Outdated
| # Check if the key is locked or not | ||
| if not check_lock(record): | ||
| if not wait_on_locks: | ||
| raise ResourceLocked( |
There was a problem hiding this comment.
This doesn't make sense: throw an exception if its not locked, and we don't want to wait on locks?
There was a problem hiding this comment.
It throws the exception only if the "wait_on_locks" is False. If it is true, it will wait on locks.
There was a problem hiding this comment.
I think you're missing the point. This reads, to me as "If not locked and we're not waiting on locks, throw an error that we're locked".
There was a problem hiding this comment.
The check_lock returns True if there is no existing lock. Otherwise, it returns False if there is an existing lock.
The intended logic is "If locked and not waiting on lock, throw an error we are locked"
There was a problem hiding this comment.
You're missing the point - I've read the code and I understand what it does. That's not where I'm confused.
My point is, in isolation, if I see if not check_lock(...), and I have no idea what check_lock does because I haven't read the function in depth yet - would I be more likely to assume that it checks if something is currently locked? Or would I be more likely to assume it checks that something isn't locked.
How does it read as English to you?
There's no negative in the function name to make me think you're going to return true if we don't have a lock. And - if that was the case - its weird to make this constructed with a double negative.
This is how you write self documenting code.
pytest_lab/locker.py
Outdated
| raise ResourceLocked( | ||
| '{} is currently locked by {}'.format(key, record.value)) | ||
| while True: | ||
| if check_lock(record): |
There was a problem hiding this comment.
You never reread the key. How do you know its current state?
pytest_lab/locker.py
Outdated
| record = self.backend.read(key) | ||
| if not record: | ||
| return True | ||
| time.sleep(0.5) |
There was a problem hiding this comment.
You don't have to poll this aggressively. You can sleep for half of ttl's length.
There was a problem hiding this comment.
Polling every half a second is from previous code.
I agree, half of ttl should be frequent enough in this scenario.
There was a problem hiding this comment.
I looked it up, you're right. Tyler made the change.
pytest_lab/locker.py
Outdated
| raise ResourceLocked( | ||
| '{} is currently locked by {}'.format(key, record.value)) | ||
| while wait_on_locks: | ||
| if check_lock(record): |
There was a problem hiding this comment.
while not check_lock(record):
passBut you really don't need to have the helper function here.
pytest_lab/locker.py
Outdated
| @@ -122,22 +122,31 @@ def __init__(self, config, backend, ttl=30): | |||
| self._thread = None | |||
| self._stop = threading.Event() | |||
|
|
|||
| def aquire(self, key, user=None): | |||
| def aquire(self, key, wait_on_locks, user=None): | |||
There was a problem hiding this comment.
Something I forgot to mention the first time around - don't change the public API here. Its a mater of global policy. This should be set in the constructor once when the plugin is initialised.
…it_on_lock option
Jane-Fan
force-pushed
the
wait-forever-lock
branch
from
256e90a to
61a7ed5
Compare
Add a key locking condition "wait-on-locks"