Dynamic workflows, activities, signals, and queries #89
Conversation
| var availStr = string.Join(", ", avail); | ||
| throw new ApplicationFailureException( | ||
| $"Activity {act.Context.Info.ActivityType} is not registered on this worker," + | ||
| $" available activities: {availStr}", |
There was a problem hiding this comment.
(Not related to this specific PR) Just a reminder that listing activities in failure details is now problematic. Server will report warning at 2KB, and truncate the Failure if exceeding 4KB (see code.
There was a problem hiding this comment.
Ug, yeah that's the size of the entire failure proto - message, stack trace, recursive causes and all. What should we do for big stack traces? What should we do for large cause chains?
Seems it's not listing activities is problematic, but large failures are problematic and I wonder if we need a holistic solution. How am I supposed to know whether I can list more or less activities just because the stack trace happens to be smaller?
There was a problem hiding this comment.
Indeed, the problem is not specifically with listing activity names. Yet, the fact that the server may penalize us on for sending the list of activity names if that exceeds some threshold means that this is not a reliable way to provide that kind of feedback to user. We'll need to find an alternative for listing registered activity/workflow/signal/queries types AND a better way to deal with activities failures containing large stack traces.
Also, I'd say that Failure should not contain stack trace for these kind of errors, if that's possible. There is no user code on the stack at this point, so nothing they should need to trace.
But really, that don't need to be resolved today.
There was a problem hiding this comment.
server may penalize us on for sending the list of activity names if that exceeds some threshold
Yeah, I need to probably put some upper bound on this
We'll need to find an alternative for listing registered activity/workflow/signal/queries types
I'd say that Failure should not contain stack trace for these kind of errors, if that's possible
It's possible, but not very transparent. As an open source library I like showing where all exceptions occur even in our own code.
| set | ||
| { | ||
| if (value != null && !value.Dynamic) | ||
| { |
There was a problem hiding this comment.
Why require that the dynamic query function has the Dynamic attribute if they are explicitely assigning to DynamicQuery? Seems redundant...
There was a problem hiding this comment.
They are different things. [WorkflowQuery] is declaration-site method made into a query. But we also offer (for DSLs and other use cases) a way to set handlers from code at runtime of the workflow. [Workflow(Dynamic = true)] is the recommended way, but DynamicQuery property is an optional runtime alternative.
There was a problem hiding this comment.
Yeah, I got that. What I'm saying is that if the user is explicitly calling the setter to set handler at runtime, why is it required that provided function do have the Dynamic attribute?
There was a problem hiding this comment.
It is not required to have that (or even have an attribute). This is a reference to a property getter that just checks the absence of the name, it is completed unrelated to the attribute. From the test for that setter:
Workflow.DynamicQuery = WorkflowQueryDefinition.CreateWithoutAttribute(
null, (string queryName, IRawValue[] args) =>
{
Events.Add($"query-{queryName}: " +
Workflow.PayloadConverter.ToValue<string>(args.Single()));
return "done";
});
| { | ||
| // If it's not null, send _all_ buffered signals. We will copy all from | ||
| // buffered, clear buffered, and send all to apply signal | ||
| var signals = bufferedSignals.ToList(); |
There was a problem hiding this comment.
This is technically incorrect, as the Workflow could change/unregister the DynamicSignal handler or register named signal handler from a signal handler, causing reentrant calls to either this loop or the one in named query handler registration. In these case, ApplySignalWorkflow might push back signals to the queue, which might scramble buffered signals relative order. See this commit for example cases.
This should instead be something like:
while (signal = bufferedSignals.tryTakeFirst()) {
ApplySignalWorkflow(signal);
}
There was a problem hiding this comment.
I think the approach you show there is more confusing but no more accurate. I intentionally take all of the signals off at handler registration time and clear the list before I even start processing them. This is very clear logic to the user setting the handler. You set a signal handler, your handler gets all buffered signals guaranteed. No maybe about it. Same for setting a named handler. It's the same problem that exists in many places when mutating a collection while iterating it and is exactly why we flush the collection before iterating on it.
With my logic, it is clear what happens when, say, you have psuedo code:
SetDynamicSignalHandler(HandleDynamicSignals)
SetSpecificSignalHandler("Signal1", HandleSignal1Signals)
The first one gets all buffered signals and the second one gets 0 buffered signals. I have documented this behavior and it is clear. But if I had to change the documentation to "start the draining process from the beginning when registered" it would be much less clear.
If you change handlers inside of some code somewhere else, doesn't affect that you chose to have all your buffered signals delivered to the original one since you know that is a registration time behavior.
There was a problem hiding this comment.
We also start all of these tasks immediately, just like if a workflow task came in with all of these signals present on it
| // Send all buffered signals that apply to this one. We are going to collect these by | ||
| // iterating list in reverse matching predicate while removing, then iterating in | ||
| // reverse | ||
| var buffered = new List<SignalWorkflow>(); |
There was a problem hiding this comment.
See related comment above
There was a problem hiding this comment.
The way signals are drained is very much by intention. It is confusing behavior to remove from this one by one.
What was changed
[Activity(Dynamic = true)]which gets called on unmatched activity in worker with singleIRawValue[]parameter[Workflow(Dynamic = true)]which gets called on unmatched workflow in worker with singleIRawValue[]parameter[WorkflowSignal(Dynamic = true)]which gets called on unmatched signal in workflow with string signal name andIRawValue[]parameter[WorkflowQuery(Dynamic = true)]which gets called on unmatched query in workflow with string query name andIRawValue[]parameterActivityExecutionContext.PayloadConverterfor use by activitiesWorkflow.PayloadConverterfor use by workflowsIPayloadConverter.ToRawValueandIPayloadConverter.ToValue<T>extensions to assist in conversion to/from raw values respectivelyRawValue/IRawValue- we make the converters otherwise available and this was needed because people may need to construct these values and the converter was only there for convenienceWorkflow.DynamicSignalandWorkflow.DynamicQueryas properties that can be set from within a workflowChecklist