-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clean up locks when EasyBuild session is cancelled with signal like SIGTERM #3291
Conversation
easybuild/tools/filetools.py
Outdated
if wait_on_lock: | ||
while os.path.exists(lock_path): | ||
print_msg("lock %s exists, waiting %d seconds..." % (lock_path, wait_on_lock), | ||
silent=build_option('silent')) | ||
time.sleep(wait_on_lock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect wait_on_lock to be the max time to wait, not how long between checks. That's what you would normally do in this situation, i.e. specify a max time to wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we need a way to limit the time you're waiting for a lock, and the semantics of --wait-on-lock
is a bit weird right now.
So, I changed things in d879cda:
--wait-on-lock
still works as before (0
(the default) implies no waiting (raise an error if lock is already there), a value implies enabling waiting with interval of specified number of seconds), but is deprecated;--wait-on-lock-limit
can be used to i) enable waiting for lock to be released, ii) specify a maximum waiting time (if set to0
is implies no waiting (raise error if lock is there), if set to-1
it implies waiting indefinitely);--wait-on-lock-interval
can be used to specify the interval for re-checking whether the lock has been released (default 60 sec.)
…it has been reached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Going in, thanks @boegel! |
cfr. #3280