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

Fix KeyError for already popped tasks #300

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Fix KeyError for already popped tasks #300

merged 1 commit into from
Nov 1, 2024

Conversation

mtweiden
Copy link
Contributor

@mtweiden mtweiden commented Oct 29, 2024

Some more complex instantiation workflows may have several instances of tasks with the same RuntimeAddress. In these cases, multiple calls to remove the task associated with the same RuntimeAddress may call Worker._tasks.pop after it has already been popped.

Adding a default pop value of None avoids raising a KeyError in these scenarios.

Fixes #301

@mtweiden mtweiden requested review from alonkukl and edyounis October 29, 2024 20:56
@alonkukl
Copy link
Contributor

I think that the current fix of adding None as a default for the pop is a good fast workaround, but race conditions between the two threads should be treated more carefully.

I will let @edyounis decide if to accept this PR or not.

@edyounis
Copy link
Member

edyounis commented Nov 1, 2024

@alonkukl What would the benefit of a mutex be for this? The outcome of the race condition shouldn't matter as long as we avoid the error, right?

@alonkukl
Copy link
Contributor

alonkukl commented Nov 1, 2024 via email

@mtweiden mtweiden merged commit 14eab3c into main Nov 1, 2024
17 checks passed
@mtweiden mtweiden deleted the default_task_pop branch November 1, 2024 21:14
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.

Race Condition in Worker._process_task_completion leading to a crash
3 participants