[Feature Request] Go over WF activation logic

[bergundy]

Currently there are some TODOs in the codebase for executing signals first and handling activation errors.
We also wait for microtasks to run between each job, make sure that's really required.

[bergundy]

Activations consist of multiple jobs.
Most jobs can run concurrently with the exception of signals, patches, and queries.

Suggestion for how node will process activations:

• process all patches (nothing async here)
• process all signals concurrently
• wait for microtasks to complete
• process remaining jobs except for queries concurrently
• wait for microtasks to complete
• process queries

Does that make sense @Sushisource @mfateev?

[mfateev]

Looks good to me. There was a request to process signals one by one for the given type. It would be nice to make it default: temporalio/sdk-java#214

[bergundy]

@mfateev I'm not sure the Java issue is relevant to us.
In node the entire Workflow runs in a single thread (concurrently) if users want to process signals sequentially they can use a channel or equivalent to buffer the signals from the signal handlers.
Does that make sense to you?

Example:

export function myWorkflow() {
  const channel = new UnboundedChannel();
  
  return {
    async execute() {
      for await (const signal of channel) {
        // handle signal
      }
    },
    signals: {
      userInput(input: any) {
        channel.send(input);
      }
    }
  };
}

[mfateev]

I think the node alternative would be not delivering a new signal until the userInput function returns.

[bergundy]

Why though?
This is pretty easy to manage in user code.

[lorensr]

It sounds like there are 2 options:

A. Signals are delivered as they're received
B. Signals are delivered sequentially—after the previous function call has returned

The Java SDK will default to B, and maybe have A as an option.

For Node, we currently only support A. We could support B or document how to ensure sequential execution, like with the code snippet above.

Support B

A con is complicating the API. Eg saying "after the arguments that you provide to your signal function, you can pass an additional options arg for Temporal to read":

workflow.signal.userInput('foo arg');
workflow.signal.userInput('foo arg', { deliverSequentially: true }); // or false, depending on what the default is

A pro is the user doesn't have to code it.

Another con is complicating our advice on how to write workflows. Currently, you can do whatever you want in a signal handler, but with B support, we'd say eg "if you have deliverSequentially on, then don't do long-running logic or sleeping in your signal handlers, because you wouldn't be able to receive signals". Versus if we don't support B, it's clear from looking at the code when a sleep() would be blocking further signal processing. Further pros of not supporting B are flexibility in how sequential is coded and clarity when looking at the signal definition (versus in the Support B case, we don't know just from looking at the handler code whether it's being called with deliverSequentially on or off—we'd have to search through the code of external systems to know).

I think I prefer not supporting B. As a Node dev, I'm used to the thought that my current function (eg an HTTP request handler) is going to be called non-sequentially, and program accordingly. It makes sense to me that a Signal, as a message from an outside system, would arrive and result in my handler being executed in the same fashion.

[bergundy]

I agree with @lorensr here.
Currently we don't implement channels, but if we did, we could add syntactic sugar for the sample above:

export function myWorkflow() {
  const signals = { userInput: new UnboundedChannel() };
  
  return {
    async execute() {
      for await (const signal of signals.userInput) {
        // handle signal
      }
    },
    signals,
  };
}