Skip to content

feat(workflow): Add support for default workflow handler and default signal handler #1038

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

Merged
merged 7 commits into from
Jan 28, 2023

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Jan 26, 2023

What changed

  • A workflow bundle can now do export default async function (...args: unknown[]): Promise<unknown> { ... } to define a default workflow handler.
  • A workflow function can now do setDefaultSignalHandler((signalName: string, ...args: unknown[]) => { ... }) to set a function that will handle signals sent for non-registered signal names.

Why

@@ -338,7 +338,7 @@ export abstract class BaseVMWorkflow implements Workflow {
await new Promise(setImmediate);
if (this.unhandledRejection) {
return {
runId: activation.runId,
runId: this.activator.info.runId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you changed this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test-workflows.ts, we set this.activator.info.runId to the title of the test, but activation.runId to test-runId.

Then, if the activation completes without issue, the completion response use this.activator.info.runId, but if it fails, it uses activation.runId.

Now, in test-workflows.ts' activate(), we do:

async function activate(t: ExecutionContext<Context>, activation: coresdk.workflow_activation.IWorkflowActivation) {
  const { workflow, runId } = t.context;
  const completion = await workflow.activate(activation);
  t.deepEqual(completion.runId, runId);   // <------ HERE
  return completion;
}

There, we compare the completion.runId to the expected runId, which corresponds to this.activator.info.runId. The sum of all of this is that if the activation fails with an exception, the t.deepEquals() will report an incorrect runId before the test code itself gets the chance to even inspect the content of the activation response itself. That makes it more difficult to locate the true reason of the failure.

This can be resolved inside test-workflows.ts instead, but it seems to me that this.activator.info.runId is really the correct thing to use, rather activation.runId.

const activator = getActivator();
const bufferedSignals = activator.bufferedSignals;
while (bufferedSignals.length) {
if (activator.defaultSignalHandler) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I see why this is correct, it's a bit confusing that we check the default handler first.
Essentially we have 2 distinct functions in one, and I think it'd be clearer if we kept the logic in the signal setter.

Copy link
Contributor Author

@mjameswh mjameswh Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficulty is that handlers (both regular and default) might get registered or unregistered from inside a signal handler. The dispatch function therefore must be reentrant-safe.

Imagine a situation where I receive signals (A, A, B, B, C, C), with following handlers:

setHandler(signalC, () => {
   setHandler(signalC, undefined);
   setHandler(signalB, () => {
       setHandler(signalB, undefined);
       setDefaultSignalHandler(() => { ... })
   })
})

This is why I always reevaluate the whole list, from the beginning, and restart every time a single signal gets dispatched.

Note that the dispatch loop is now O(N^2), but we don't expect N to grow anything big.

A possible optimisation here would be to have a "signal handler changed" counter, and only restart the loop if that counter changed while dispatching a signal. That would bring the dispatch function down to O(N) in most cases, but I don't think that the potential performance gain in extremely rare cases justify the increased complexity that this solution involves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I didn't account for registration of signal handlers inside signal handlers.
We'll need to check this to ensure ordering.
It's probably worth moving this code to the activator since it's relying on some of its internals.

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.
Had some comments that should be easily addressable.

@cretz
Copy link
Member

cretz commented Jan 27, 2023

Didn't read review, but weren't there some concerns that by using export default which is a common thing in JS that we'd be over-encouraging the use of dynamic workflow handlers? Did we want to consider making it a bit more unwieldy and/or requiring maybe an explicit opt-in boolean at registration time saying "I am intentionally using dynamic workflow" and throw if default exported function is present otherwise?

@bergundy
Copy link
Member

Yes, there's some concern about export default being common in JS.
We'd considered using a different convention but this seems like the best option since we don't want to prevent other strings, we can't use symbols, and default is reserved in JS.
I think requiring opt-in will be safer and also a bit more annoying but I can see a point for preferring safety especially when adding this convention to a mature SDK.

- Be more protective on supplied args, more efficient at use time
- Move dispatchBufferedSignals to activator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Catch-all signal handler
3 participants