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

Task lock should be non-local #333

Merged
merged 5 commits into from
Apr 24, 2024
Merged

Task lock should be non-local #333

merged 5 commits into from
Apr 24, 2024

Conversation

thomasst
Copy link
Member

@thomasst thomasst commented Apr 24, 2024

Analyzing the thread safety of the heartbeat function in the sync executor:

  • Redis-py should by threadsafe by default unless single_connection_client is given, which is false by default.
  • When doing a heartbeat() we update the task timestamp, renew any task locks and any queue lock.
  • The queue lock (Semaphore) does not care about threads so we can therefore renew it from a different thread.
  • Redis locks, which we use for locking tasks, are thread-local by default, we therefore need to set thread_local=False.
  • We use Redis locks in a couple other places but these are not being accessed by the heartbeat thread.

Also:

  • Improved heartbeat so we don't need to do a heartbeat if the task already completed after the wait, and added exception handling.
  • Fixed test to give it a bit more time to start (had some failures locally) + have at least one task & queue lock that should be renewed.

- Don't heartbeat if we're about to exit anyways
- Remove unnecessary call to is_set
- Handle exceptions in heartbeat just in case
In rare cases I saw the test fail locally.
@thomasst thomasst force-pushed the fix-thread-local-lock branch from 045c507 to b86d469 Compare April 24, 2024 11:32
@thomasst thomasst requested a review from AlecRosenbaum April 24, 2024 11:36
@thomasst thomasst merged commit 6626754 into master Apr 24, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants