Skip to content
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

Add default update handler #1640

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add default update handler #1640

wants to merge 6 commits into from

Conversation

THardy98
Copy link
Contributor

@THardy98 THardy98 commented Mar 7, 2025

Added ability to register a default update handler. The default handler will handle updates with names that have no registered handler.

  1. Part of [Feature Request] Support dynamic workflows, activities, signals, queries, and updates #1015

  2. How was this tested:
    Added a couple activation tests with some comments describing the semantics of update code.

  3. Any docs updates needed?
    Maybe

@THardy98 THardy98 requested a review from a team as a code owner March 7, 2025 00:47
@THardy98 THardy98 changed the title Default updates Add default update handler Mar 7, 2025
id,
protocolInstanceId,
name,
input: toPayloads(defaultPayloadConverter, input),
Copy link
Contributor

@mjameswh mjameswh Mar 7, 2025

Choose a reason for hiding this comment

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

Shouldn't the second argument of toPayloads be an array? If so, the input argument should be unknown[] instead of just unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but input is a rest parameter in toPayloads, its a variadic function. I've changed it anyways for consistency.

1. setHandler for updateA
- this is all synchronous code until we run `UpdateScope.updateWithInfo(updateId, name, doUpdateImpl)`,
which calls `doUpdateImpl` which is promise/async, so...
Copy link
Contributor

@mjameswh mjameswh Mar 7, 2025

Choose a reason for hiding this comment

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

To be exact, it is sync until the point where we actually call the update handler (i.e. the execute(input) call in doUpdateImpl(). By specification, the update validation must be executed sync, and the workflow handler function must be called immediately after. Otherwise, the workflow's internal state could be modified between the validation and the execute functions, and the validation result could be no longer valid by the time the execute function is called.

And in the specific case of this Workflow, the update handler is purely sync (i.e. there is no promise awaited on that code path), so it will also execute synchronously.

Copy link
Contributor Author

@THardy98 THardy98 Mar 7, 2025

Choose a reason for hiding this comment

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

I'm confused.

By specification, the update validation must be executed sync, and the workflow handler function must be called immediately after. Otherwise, the workflow's internal state could be modified between the validation and the execute functions, and the validation result could be no longer valid by the time the execute function is called.

This makes sense to me.

But how can the code be sync up until we call the execute call?

  /** Alias to `new UpdateScope({ id, name }).run(fn)` */
  static updateWithInfo<T>(id: string, name: string, fn: () => Promise<T>): Promise<T> {
    return new this({ id, name }).run(fn);
  }

UpdateScope.updateWithInfo(updateId, name, doUpdateImpl)

This is the async call at the end of doUpdate, it returns a promise.
doUpdateImpl itself is an async function as well.
How could these async calls guarantee sync execution till we execute the handler?

This is irrespective of the update handler code (async or not).

1. setHandler for updateA
- this is all synchronous code until we run `UpdateScope.updateWithInfo(updateId, name, doUpdateImpl)`,
which calls `doUpdateImpl` which is promise/async, so...
2. queue doUpdateImpl for A on node event queue: [doUpdateImplA]
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS, in the case of this specific workflow, doUpdateImplA will not get queued on node's event queue because there's no promise that is awaited from that update handler. Same for updateB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doUpdateImplA is async though, so I thought differently. Maybe we should coalesce this conversation into the other comment.

@THardy98 THardy98 requested a review from mjameswh March 7, 2025 22:55
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.

2 participants