-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adds circuit breaker when a task depends on itself #13882
Conversation
else: | ||
wait_for = [] | ||
future = say_hello.submit(name=f"Person {i}", wait_for=wait_for) | ||
greeting_queue.append(future) |
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.
Hmm, so the only way this can happen is if the caller changes the wait_for
iterable after submitting the task. If we just make sure to make our own copy of wait_for
, doesn't this problem go away?
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.
@desertaxle pointed out copying could cause memory issues. Maybe better to start simple and copy if we need to later?
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.
It wouldn't be a copy of the futures, just a copy of the iterable the caller gave us so that it's kind of "causally locked in" at that point (I'm just thinking literally wait_for = list(wait_for)
. I can't help but think there are other classes of bugs lurking if we let this list of futures be mutable after it's passed to the engine.
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.
This also happens if futures are passed in parameters which can be deeply nested and would require a deepcopy
to prevent subsequent modification from causing deadlocks. This probably won't happen often, so I'd rather raise in the unlikely chance this happens then incur a memory penalty in all other cases.
Co-authored-by: Chris Guidry <chris.g@prefect.io>
Adds a circuit breaker to raise when a task depends on itself which would otherwise cause a flow to hang indefinitely.
Closes #13874
Example
This flow sneakily has a task that depends on itself by updating an array in place:
Running this flow with this PR will raise an error like this:
Checklist
<link to issue>
"maintenance
,fix
,feature
,enhancement
,docs
.For documentation changes:
mint.json
for files that are removed or renamed.For new functions or classes in the Python SDK:
docs/mint.json
navigation.