-
Notifications
You must be signed in to change notification settings - Fork 33
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
Run wait-condition callbacks in workflow context #242
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -648,38 +648,58 @@ protected override bool TryDequeue(Task task) | |
private void RunOnce(bool checkConditions) | ||
{ | ||
// Run as long as we have scheduled tasks | ||
// TODO(cretz): Fix to run as long as any tasks not yielded on Temporal | ||
while (scheduledTasks.Count > 0) | ||
{ | ||
while (scheduledTasks.Count > 0) | ||
// Run all tasks until empty | ||
RunAllTasks(); | ||
|
||
// If there are any conditions, schedule a new condition-check task and the run all | ||
// tasks again. This is a scheduled task instead of just run inline because it needs | ||
// to be in the workflow context (i.e. current task scheduler) so the `Workflow` | ||
// methods work properly. | ||
if (checkConditions && conditions.Count > 0) | ||
{ | ||
// Pop last | ||
var task = scheduledTasks.Last!.Value; | ||
scheduledTasks.RemoveLast(); | ||
scheduledTaskNodes.Remove(task); | ||
_ = QueueNewTaskAsync(CheckConditionsAsync); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my own understanding in dotnet, Do we detect if the user tries to call any illegal APIs in the wait condition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't and we probably should. We have been mostly counting on the fact that it's not async to prevent commands, but there are commands like upsert search attribute that are not async. This is actually a gap for queries and update validators too. I have opened #243. |
||
RunAllTasks(); | ||
} | ||
} | ||
} | ||
|
||
// This should never return false | ||
if (!TryExecuteTask(task)) | ||
{ | ||
logger.LogWarning("Task unexpectedly was unable to execute"); | ||
} | ||
if (currentActivationException != null) | ||
{ | ||
ExceptionDispatchInfo.Capture(currentActivationException).Throw(); | ||
} | ||
private void RunAllTasks() | ||
{ | ||
while (scheduledTasks.Count > 0) | ||
{ | ||
// Pop last | ||
var task = scheduledTasks.Last!.Value; | ||
scheduledTasks.RemoveLast(); | ||
scheduledTaskNodes.Remove(task); | ||
|
||
// This should never return false | ||
if (!TryExecuteTask(task)) | ||
{ | ||
logger.LogWarning("Task unexpectedly was unable to execute"); | ||
} | ||
if (currentActivationException != null) | ||
{ | ||
ExceptionDispatchInfo.Capture(currentActivationException).Throw(); | ||
} | ||
} | ||
} | ||
|
||
// Collect all condition sources to mark complete and then complete them. This | ||
// avoids modify-during-iterate issues. | ||
if (checkConditions) | ||
private Task CheckConditionsAsync() | ||
{ | ||
try | ||
{ | ||
foreach (var source in conditions.Where(t => t.Item1()).Select(t => t.Item2)) | ||
{ | ||
var completeConditions = conditions.Where(tuple => tuple.Item1()); | ||
foreach (var source in conditions.Where(t => t.Item1()).Select(t => t.Item2)) | ||
{ | ||
source.TrySetResult(null); | ||
} | ||
source.TrySetResult(null); | ||
} | ||
} | ||
catch (Exception e) | ||
{ | ||
currentActivationException = e; | ||
} | ||
return Task.CompletedTask; | ||
} | ||
|
||
private void AddCommand(WorkflowCommand cmd) | ||
|
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.
So git diff makes these changes look a bit scarier than they are. We just moved the run-all-task loop into a separate method, then enqueue the condition checker task instead of invoking it inline (issuing a run-all-task again for that new task).