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

[API Breaking] Separate wait_for into two methods: wait_for_any and wait_until #174

Merged
merged 4 commits into from
May 5, 2022

Conversation

jeffschoner-stripe
Copy link
Contributor

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 to wait_for, this signature became much more complicated, taking both a splat of futures and a conditional block. Note this is API breaking because the wait_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 of wait_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,`
signal = nil
workflow.on_signal |name, input| do
  signal = input
end

timer = workflow.start_timer(60)
workflow.wait_until { !signal.nil? || timer.finished? }
# examine state here

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.

return
end

def wait_until(&unblock_condition)
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@jeffschoner-stripe jeffschoner-stripe force-pushed the separate-wait-for branch 3 times, most recently from 4eddfc3 to e095618 Compare April 20, 2022 17:04
Copy link
Contributor

@antstorm antstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@DeRauk DeRauk merged commit aa30be1 into coinbase:master May 5, 2022
@jeffschoner-stripe jeffschoner-stripe deleted the separate-wait-for branch October 26, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants