feat(sdk): workflow interceptors - #1401
Conversation
7b18607 to
358c765
Compare
| impl ContinueAsNewInput { | ||
| pub(crate) fn new(input: Box<dyn Any>, options: ContinueAsNewOptions) -> Self { | ||
| Self { | ||
| decoded: DecodedInput::new(Some(input), HashMap::new()), |
There was a problem hiding this comment.
How can user specify headers? Needs to be extracted from options
There was a problem hiding this comment.
Removed the headers field from continue as new opts so headers must be set in an interceptor now. Aligns with TS/.NET.
| ) -> Result<Box<dyn WorkflowInstance>, anyhow::Error> { | ||
| if !input.workflow_interceptor_factories.is_empty() { | ||
| bail!("Native workflow interceptors cannot be used with WASM workflow components"); | ||
| } |
There was a problem hiding this comment.
This seems like the wrong place to fail - we will fail every time we try to execute a workflow defined in WASM rather than just failing on worker startup, which seemingly we could do and would be faster.
However, I'm not sure why this needs to be an error. If a user defines interceptors in the WASM bundle, I think we can use those and just document that native ones do not apply to WASM workflows?
I'm also not 100% clear on why native ones can't be applied to WASM workflows. I guess just extra back-and-forth glue? (To be clear, I don't object, just want to clarify).
There was a problem hiding this comment.
However, I'm not sure why this needs to be an error. If a user defines interceptors in the WASM bundle, I think we can use those and just document that native ones do not apply to WASM workflows?
You're right, no need to error in this scenario.
I guess just extra back-and-forth glue?
Correct, this has been in development for long enough and the PR is large enough that I didn't feel tackling this here was necessary.
9020ef7 to
b87ff2e
Compare
| "--release", | ||
| "--target", | ||
| "wasm32-unknown-unknown", | ||
| "--target-dir", |
There was a problem hiding this comment.
Not strictly necessary, but I have a custom target dir setup on my machine and allows me to run WASM tests locally without overriding it.
| .boxed_local() | ||
| Ok(Box::new(output) as Box<dyn WorkflowOutputValue>) | ||
| }; | ||
| ConstructionBlockedFuture::new(base_ctx, future.boxed_local()).boxed_local() |
There was a problem hiding this comment.
We want to ensure that if there is a sync handler with no interceptors, it is driven to completed before starting to poll an async handler.
| impl ContinueAsNewInput { | ||
| pub(crate) fn new(input: Box<dyn Any>, options: ContinueAsNewOptions) -> Self { | ||
| Self { | ||
| decoded: DecodedInput::new(Some(input), HashMap::new()), |
There was a problem hiding this comment.
Removed the headers field from continue as new opts so headers must be set in an interceptor now. Aligns with TS/.NET.
|
|
||
| pub(crate) enum NexusResultFuture { | ||
| Raw(Shared<WFCommandFut<NexusOperationResult, ()>>), | ||
| Intercepted(Shared<LocalBoxFuture<'static, NexusOperationResult>>), |
There was a problem hiding this comment.
On second thought I removed this entirely since the final shape of calling nexus operations hasn't stabilized.
| } | ||
|
|
||
| /// Creates a fresh interceptor collection for each workflow instance. | ||
| pub trait WorkflowInterceptorFactory: Send + Sync + 'static { |
There was a problem hiding this comment.
If this took some param that provided context about the workflow
Meant to do that, but totally forgot which did make it pointless.
Updated to now take a vec of WorkflowInterceptorConstructors which are wrappers around Fn(&WorkflowContextView) -> WorkflowInterceptor. Only have a named type so end users don't need to deal with the boxing of closures and the constructed interceptor.
| }; | ||
| ActivityFut::running( | ||
| LATimerBackoffFut::new(activity.name().to_string(), payloads, opts, self.clone()), | ||
| let future = LATimerBackoffFut::new(activity_type, payloads, headers, opts, self.clone()); |
| ) -> Result<Box<dyn WorkflowInstance>, anyhow::Error> { | ||
| if !input.workflow_interceptor_factories.is_empty() { | ||
| bail!("Native workflow interceptors cannot be used with WASM workflow components"); | ||
| } |
There was a problem hiding this comment.
However, I'm not sure why this needs to be an error. If a user defines interceptors in the WASM bundle, I think we can use those and just document that native ones do not apply to WASM workflows?
You're right, no need to error in this scenario.
I guess just extra back-and-forth glue?
Correct, this has been in development for long enough and the PR is large enough that I didn't feel tackling this here was necessary.
Sushisource
left a comment
There was a problem hiding this comment.
I find the complexity of the whole intercepted futures system to be a bit concerning but I don't have any good alternative suggestions off the top of my head
e492cce to
c24f347
Compare
What was changed
WorkflowOutputValuethat allows interceptors to inspect/modify outputs before serialization.continue_as_newno longer takes reference to input since interceptors might modify/swap the input. In retrospect taking a reference was also awkward IMO.WorkflowInterceptortrait and update workflow machinery to use interceptors.add_workflow_interceptor_factory/WorkflowInterceptorFactorywhich constructs an named array of workflow interceptors.WorkflowInterceptorFuturea named future that is the expected future to be produced by interceptor futures. We could exposeLocalBoxFuturedirectly instead if we're willing to expose that 3rd party type as an external API type.WorkflowInterceptorInterceptors look like the following:
They take a context with a subset of functionality of a workflow context, input for the operation and a
nextcontinuation that calls the next interceptor in the chain. They are always sync forcing users to do any async work in aWorkflowInterceptorFuture.Why?
The changes to the workflow machinery and the "construction" polling are to allow the interceptor chains to not delay command creation/handler execution if there isn't any async work. This was especially apparent with sync handlers that were eagerly executed previously. Without this special handling, the existence of an interceptor chain would at least one yield point before the execution of the handler/command creation. The construction poll is our way of ensuring we drive the chain as forward as possible.
An alternative for this would be to make the interceptor API where
Nextwould have some explicit.thenmethods to remove any yield points. I felt this diverged too much from all other SDKs and was generally pretty awkward to work with.A few other decisions
Checklist
Closes [Feature Request] Workflow interceptors #1139
How was this tested:
See integration tests.
Any docs updates needed?
Once released should add interceptors page for the Rust SDK docs.