-
Notifications
You must be signed in to change notification settings - Fork 131
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
New docs section for canceled-context in workflows, other minor cleanups #1134
Conversation
Pull Request Test Coverage Report for Build fcc0307a-17f1-4226-a3a7-ff6129226ebd
💛 - Coveralls |
- workflow.Await | ||
- workflow.Sleep | ||
- workflow.Timer | ||
|
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.
signaling and RequestCancelExternalWorkflow both do not mention CanceledError... but they seem like things that should probably no-op on cancel. anyone know what they do?
So the error handling code would look like: | ||
|
||
err := workflow.ExecuteActivity(ctx, YourActivityFunc).Get(ctx, nil) | ||
switch err := err.(type) { | ||
case *error.CustomError: |
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.
we have no error
package ¯\_(ツ)_/¯
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.
Thanks for fixing. Even if we have, I think using workflow.XyzError is more friendly than
error.XyzErrorbecause there will be a lot of
error` pkg loaded into the IDE.
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.
yeah, "error" and "errors" are kinda risky in that way. lots of chances for collisions -> annoying manual aliases.
|
||
Async/Manual Activity Completion |
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.
/
prevents godoc from interpreting it as a header
workflow/doc.go
Outdated
For exact behavior, make sure to read the documentation on functions that you are calling. | ||
|
||
As an incomplete summary, these actions will all fail immediately (or when the activity completes, if WaitForCancellation | ||
is true), and the associated error returns (possibly within a Future) will be a workflow.CanceledError: |
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.
or when the activity completes, if WaitForCancellation is true
Is this the default behavior? I tested this before seems like so. Maybe we should remove activity from this summary and make it as a special case.
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.
default behavior is false, so it does not wait.
are you thinking to just drop the exception note (or say "read executeactivity for more details"), or put it in its own list since it (and maybe executechildworkflow? I'm not sure how it completes either) is kinda unique?
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.
default behavior is false, so it does not wait.
Interesting, looks like Java client is different. I tested in
uber/cadence-java-samples#47
that it will wait for completion.
also uber/cadence-java-samples#48 for long running activity activity will get canceled right way when reporting heartbeat.
Yeah I believe pulling activity out is better to read. if having a separate paragraph. It has different cases than other. Using ()
is not too friendly to understand.
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.
some minor comments, otherwise LGTM
workflow/doc.go
Outdated
Activities have configurable cancellation behavior. For workflow.ExecuteActivity and workflow.ExecuteLocalActivity, | ||
see the activity package's documentation for details. | ||
|
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.
@longquanzheng seems better?
workflow/doc.go
Outdated
As an incomplete summary, these actions will all fail immediately, and the associated error returns (possibly within | ||
a Future) will be a workflow.CanceledError: | ||
|
||
- workflow.ExecuteChildWorkflow (pending a bugfix, see #1234) |
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.
gotta get an in-test repro in place, then I'll file an issue and investigate fixes.
…ups (#1134) Canceled-context documentation was strewn about in individual function documentation, though it's a fairly important concept that needs to be understood. In particular, users are sometimes under the impression that "all things which accept a context will error", which can lead to nasty surprises like `selector.Select(ctx)`'s blocking behavior (causing a deadlocked workflow). Selector's docs have been improved in #1131, and this will hopefully make its behavior easier to learn before it is needed. And, since I noticed some small other things while writing this chunk of docs, I've rolled those changes into here as well.
Canceled-context documentation is currently strewn about in individual function documentation, though it's a fairly important concept that needs to be understood.
In particular, users sometimes are under the impression that "all things which accept a context will error", which can lead to nasty surprises like
selector.Select(ctx)
's blocking behavior (causing a deadlocked workflow). Selector's docs have been improved in #1131, and this will hopefully make its behavior easier to learn before it is needed.And, since I noticed some small other things while writing this chunk of docs, I've rolled those changes into here as well.