-
Notifications
You must be signed in to change notification settings - Fork 96
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
[API Breaking] Separate wait_for into two methods: wait_for_any and wait_until #174
Conversation
return | ||
end | ||
|
||
def wait_until(&unblock_condition) |
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.
The change makes sense and clarifies a lot of things, thank you @jeffschoner-stripe . One thing I'm not sure — why not use the original wait_for
here? My brain thinks that wait_until
suggests a time argument, but it might be just me :)
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.
I had originally called this wait_for_condition
but the until language seemed more similar to the Ruby until
keyword.
I didn't want to preserve wait_for
because if someone is using that with a block today, removing the block from the signature is not breaking. Their code would just start silently ignoring the block condition. For example, if they were calling wait_for(timeout_timer_future) { some_variable }
, removing the block argument from the function would have the effect of just waiting for the timeout and ignoring the condition.
However, if wait_for
goes away entirely, it will immediately break their code once upgrading to the new version. I could also do leave the wait_for
there but mark it as deprecated or leave it there to only raise an error explaining that the new functions should be used instead. What do you think? Unfortunately, I don't have a clear picture of who may be using these APIs today.
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.
I suppose I could also keep calling it wait_for
and have it only take the condition block. That would be breaking, and I could add an explanatory comment
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.
I think approach here is clearer. It seems better to have the method completely go away, so that any breakage would be resolved by looking for this PR or by reading comments on workflow context.
lib/temporal/workflow/context.rb
Outdated
dispatcher.register_handler(future.target, Dispatcher::WILDCARD) do | ||
# Because any of the futures can resume the fiber, ignore any callbacks | ||
# from other futures after unblocking has occurred | ||
if blocked |
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 has lost the future.finished?
check, which means that adding any other dispatch calls to this target (like on started
) will break this behaviour.
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.
Good catch. I'll add a test for this case too.
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.
I've added this back in the latest commit, as well as thorough unit specs for wait_until
, wait_for_any
and wait_for_all
4eddfc3
to
e095618
Compare
e095618
to
5e7bd9d
Compare
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.
Looks good!
This is a follow up to #111 that improves API ergonomics of the use of wait_for with a block condition.
The wait_for method on workflow context started as a way to wait on a single future to complete. It was rarely called directly because activity or timer futures have their own
.wait
methods that called through to this method. With the introduction of a conditional block towait_for
, this signature became much more complicated, taking both a splat of futures and a conditional block. Note this is API breaking because thewait_for
method is going away. This method should rarely be called for activities and timers since futures already have an equivalent wait method. The use ofwait_for
with a condition block or with wait-for-any semantics will need to be rewritten using the new methods.With this split, it is now two methods:
wait_for_any
which takes a splat of futures, and blocks workflow progression until at least one future is completed. This is similar to the already existing wait_for_all.wait_until
which takes a block, and blocks until that block evaluates to true. Complex combinations of futures and workflow state can be combined into a single conditional block. For example, waiting on a timer firing or a signal being received can be done with something like,`Test plan
Existing tests pass. There is no new functionality or behavior here.
Example test cases have been simplified by removing parts that combined waiting on a future and a conditional block in a single
wait_for
call.