Skip to content
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

use threading.Lock.acquire's timeout for backpressure rather than sleep #650

Merged
merged 1 commit into from
Feb 11, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions sh.py
Original file line number Diff line number Diff line change
Expand Up @@ -2491,16 +2491,12 @@ def is_alive(self):
# so essentially what we're doing is, using this lock, checking if
# we're calling .wait(), and if we are, let .wait() get the exit code
# and handle the status, otherwise let us do it.
acquired = self._wait_lock.acquire(False)
#
# Using a small timeout provides backpressure against code that spams
# calls to .is_alive() which may block the main thread from acquiring
# the lock otherwise.
acquired = self._wait_lock.acquire(timeout=0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I still had to set this back down to:

Suggested change
acquired = self._wait_lock.acquire(timeout=0.1)
acquired = self._wait_lock.acquire(timeout= 0.00001)

as it was before e9511de.

Given we're using the acquire provided timeout, is that fine now?

if not acquired:
# this sleep here is a hack. occasionally, is_alive can get called so
# rapidly that it appears to not let the wait lock be acquired in the main
# thread, which is attempting to call wait(). by introducing a tiny sleep
# (ugh), this seems to prevent other threads from equally attempting to
# acquire the lock. TODO find out if this is a general python bug
# if we don't do this, if we're unlucky, some commands may hang for a
# second before terminating, due to their threads spamming is_alive() calls.
time.sleep(0.1)
if self.exit_code is not None:
return False, self.exit_code
return True, self.exit_code
Expand Down