[Fix] Non-started activity cancellation #125
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| # with the intent to propagate to a workflow | ||
| class ActivityException < ClientError; end | ||
|
|
||
| # Represents cancellation of a non-started activity |
Contributor
There was a problem hiding this comment.
Since cancels can also happen at a heartbeat, should we unify this case with that case? It's not ideal that people would have to deal with both depending on the timing of when the cancellation comes in.
If that's not possible, should name it as ActivityCanceledBeforeStart?
Contributor
Author
There was a problem hiding this comment.
There's nothing preventing other to re-use the Temporal::ActivityCancelled error for heartbeat cancellations. It's just that a heartbeat cancellation is no different from any other failure raised by an activity. I think a more interesting question here is — should we provide a #cancel! API for the activity context that would raise that error?
Contributor
There was a problem hiding this comment.
Oh, I forgot that in this SDK, heartbeat cancel doesn't throw, it returns a boolean.
In our wrapper SDK (and in the java SDK), it will throw. We think this is the best way to make sure developers don't drop cancellations.
| context 'when event is ACTIVITY_CANCELED' do | ||
| let(:raw_event) { Fabricate(:api_activity_task_canceled_event) } | ||
|
|
||
| it 'sets type to timer' do |
| context 'when event is ACTIVITY_TASK_CANCEL_REQUESTED' do | ||
| let(:raw_event) { Fabricate(:api_activity_task_cancel_requested_event) } | ||
|
|
||
| it 'sets type to cancel_timer_request' do |
Contributor
There was a problem hiding this comment.
©️ 🍝 here as well
gvieira
pushed a commit
to clearbit/temporal-ruby
that referenced
this pull request
Dec 21, 2021
* Pass config to the error handler instead of using the global config * Fix example tests * Make the config property in the workflow context publicly readable * [Fix] Retryer GRPC error lookup (coinbase#109) * Fix issue with GRPC error lookup in Retryer * Rename spec file for retryer to contain _spec * [Feature] Add id and domain to workflow context's metadata (coinbase#110) * Start syncing id and domain on workflow context metadata * Fixed tests Co-authored-by: DeRauk Gibble <derauk.gibble@coinbase.com> * Explicit docker-compose project name (coinbase#114) * Add YARD documentation for Temporal::Client (coinbase#113) * Add YARD documentation for Temporal::Client * Add yard gem * Fix @option tag * Typo fix * Add signal arguments to start_workflow (support for signal_with_start) (coinbase#112) * Add signal arguments to start_workflow (to support signal_with_start) * Move signal arguments to the options hash * PR feedback * Fix merge error * Extend #wait_for to take multiple futures and a condition block (coinbase#111) * Differentiate TARGET_WILDCARD and WILDCARD, allow comparison with EventTarget objects (coinbase#118) * Turn off schedule_to_start activity timeout by default (coinbase#119) * Separate options from keyword args in #start_workflow (coinbase#117) * Separate options from keyword args in #start_workflow * fixup! Separate options from keyword args in #start_workflow * Surface additional workflow metadata on workflow context (coinbase#120) * Refactor metadata generation * Make task queue available on workflow metadata, add example test * Expose workflow start time metadata * Add memos (coinbase#121) * Add describe_namespace (coinbase#122) * Add describe_namespace * Feedback * Improve header serialization and propagation (coinbase#124) * [Fix] Non-started activity cancellation (coinbase#125) * Fix event target map entry for ACTIVITY_CANCELED event * Fix cancellation of a non-started activity * fixup! Fix event target map entry for ACTIVITY_CANCELED event Co-authored-by: DeRauk Gibble <derauk.gibble@coinbase.com> Co-authored-by: DeRauk Gibble <derauk@gmail.com> Co-authored-by: Anthony Dmitriyev <antstorm@gmail.com> Co-authored-by: nagl-stripe <86737162+nagl-stripe@users.noreply.github.com> Co-authored-by: jeffschoner-stripe <63118764+jeffschoner-stripe@users.noreply.github.com> Co-authored-by: Drew Hoskins <37816070+drewhoskins-stripe@users.noreply.github.com>
| sleep 0.5 | ||
| Temporal.signal_workflow(LongWorkflow, :CANCEL, workflow_id, run_id) | ||
|
|
||
| result = Temporal.await_workflow_result( |
Contributor
There was a problem hiding this comment.
This is always failing for me, locally
1) Activity cancellation cancels a running activity
Failure/Error: expect(result).to be_a(LongRunningActivity::Canceled)
expected #<Temporal::ActivityCanceled: ACTIVITY_ID_NOT_STARTED> to be a kind of LongRunningActivity::Canceled
# ./spec/integration/activity_cancellation_spec.rb:19:in `block (2 levels) in <top (required)>'
calum-stripe
pushed a commit
to calum-stripe/temporal-ruby
that referenced
this pull request
Mar 28, 2023
…oner-WOFLO-1216 Support terminate-if-running workflow ID reuse policy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an activity cancellation is requested before the activity had a chance to start Temporal will issue an
ACTIVITY_CANCELEDevent. This PR fixed handling of this event.Tested: