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: Pass back real result for single task chains #6411
Conversation
Ah, I need to pull one of the integrations tests across from that PR as well. This is the change that makes it not time out! |
This pull request fixes 1 alert when merging 8b4250d into 735f167 - view on LGTM.com fixed alerts:
|
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.
Can we add an integration test for this?
8b4250d
to
f0242da
Compare
Yep, |
This pull request introduces 1 alert when merging f0242da into 735f167 - view on LGTM.com new alerts:
|
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.
apart from the better naming, looks good
When chains are delayed, they are first frozen as part of preparation which causes the sub-tasks to also be frozen. Afterward, the final (0th since we reverse the tasks/result order when freezing) result object from the freezing process would be passed back to the caller. This caused problems in signaling completion of groups contained in chains because the group relies on a promise which is fulfilled by a barrier linked to each of its applied subtasks. By constructing two `GroupResult` objects (one during freezing, one when the chain sub-tasks are applied), this resulted in there being two promises; only one of which would actually be fulfilled by the group subtasks. This change ensures that in the special case where a chain has a single task, we pass back the result object constructed when the task was actually applied. When that single child is a group which does not get unrolled (ie. contains more than one child itself), this ensures that we pass back a `GroupResult` object which will actually be fulfilled. The caller can then await the result confidently!
f0242da
to
bd6c6bd
Compare
I went with |
This pull request introduces 1 alert when merging bd6c6bd into 735f167 - view on LGTM.com new alerts:
|
Description
When chains are delayed, they are first frozen as part of preparation
which causes the sub-tasks to also be frozen. Afterward, the final (0th
since we reverse the tasks/result order when freezing) result object
from the freezing process would be passed back to the caller. This
caused problems in signaling completion of groups contained in chains
because the group relies on a promise which is fulfilled by a barrier
linked to each of its applied subtasks. By constructing two
GroupResult
objects (one during freezing, one when the chain sub-tasksare applied), this resulted in there being two promises; only one of
which would actually be fulfilled by the group subtasks.
This change ensures that in the special case where a chain has a single
task, we pass back the result object constructed when the task was
actually applied. When that single child is a group which does not get
unrolled (ie. contains more than one child itself), this ensures that we
pass back a
GroupResult
object which will actually be fulfilled. Thecaller can then await the result confidently!