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

Run wait-condition callbacks in workflow context #242

Merged
merged 2 commits into from
May 8, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented May 8, 2024

What was changed

Changed logic to make the wait condition callbacks a task in so it is run within the workflow context so the user can access things on the Workflow static class

Checklist

  1. Closes [Bug] WaitConditionAsync is run outside of Workflow context #240

@cretz cretz requested a review from a team May 8, 2024 16:49
Copy link
Member Author

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).

var task = scheduledTasks.Last!.Value;
scheduledTasks.RemoveLast();
scheduledTaskNodes.Remove(task);
_ = QueueNewTaskAsync(CheckConditionsAsync);

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@cretz cretz merged commit 5cd6f59 into temporalio:main May 8, 2024
7 checks passed
@cretz cretz deleted the wait-condition-context branch May 8, 2024 22:29
@cretz
Copy link
Member Author

cretz commented Jun 3, 2024

This caused a bug. Now conditions in coroutines depending on each other may not move forward because conditions are not re-run upon task resumption.

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.

[Bug] WaitConditionAsync is run outside of Workflow context
3 participants