Skip to content

[Feature Request] Add flag to disable signal/update reordering #640

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

Closed
Sushisource opened this issue Nov 20, 2023 · 4 comments
Closed

[Feature Request] Add flag to disable signal/update reordering #640

Sushisource opened this issue Nov 20, 2023 · 4 comments
Labels
enhancement New feature or request Rust SDK Issues about or asking for Rust SDK release

Comments

@Sushisource
Copy link
Member

As described in the docstring update in #639, the reordering of signals/queries to come first is more a byproduct of language runtime specifics than anything else.

Languages that can control the iteration of routines/tasks/whatever, don't need this reordering, and in fact it prevents one specific case that seems like it should work from working:

Say your history has something like:

...
activity completed
signaled
WFT Sched & Start

If I reorder jobs such that lang sees [signal, activity resolve]

Then code that looks like this:

fn signal_handler() {
    if that_activity_future.is_completed() {
        // do a thing
    }
}

Will not enter the if condition, since the signal handler is invoked first. However it seemingly should, according to history, since the activity is completed before the signal is received.

Rust SDK should make use of the more accurate behavior, since it can. Ideally, Python and .NET would too since they have this control, though that could potentially represent a breaking change. If we could confirm it's nonbreaking, we could do it, but not exactly important.

@Sushisource Sushisource added enhancement New feature or request Rust SDK Issues about or asking for Rust SDK release labels Nov 20, 2023
@bergundy
Copy link
Member

I generally agree that reordering is not strictly needed, it's there for a couple of reasons (there are probably more cases I missed):

  1. late signal handler registration - in all languages we support late (dynamic) registration, this is the main form of registration in languages like TS where handlers are registered in the body of the main workflow function. We want the signals handlers to be called as soon as they're registered as shown here.
  2. evaluation of conditions - conditions, a.k.a Workflow.await in some languages, are evaluated at key points in the activation. Conditions may have timeouts (as shown below). We prefer letting the condition function win the race with a timer. Whether or not that is the right decision can be debated but that's a choice we made.
export async function conditionRacer(): Promise<void> {
  let blocked = true;
  setHandler(unblockSignal, () => void (blocked = false));
  const unblocked = await condition(() => !blocked, '1s');
  assert.true(unblocked);
}

@Sushisource
Copy link
Member Author

1 is accomplished via buffering, since it's a problem whether you re-order or not. If you re-order and try to process a signal that isn't registered yet, you need to buffer it same as if you didn't reorder and try to process the signal as you go.

2 Which one wins here (assuming we're saying signal and timer for the timeout fired in the same task) would depend, if you're not reordering, on which one landed in history first - that makes perfect sense to me intuitively speaking. But yes, it's not something you could change about the existing SDKs unfortunately.

@cretz
Copy link
Member

cretz commented Nov 27, 2023

Can we confirm Java SDK behavior here?

Also, if we allow relying on the ordering of non-signal/update and signal/update event processing in a single task, I think switching signal registration from predefined to first-line-in-workflow defined but left everything else the same could represent a non-deterministic change, correct? Maybe that's ok to be a non-deterministic change.

Python and .NET would too since they have this control, though that could potentially represent a breaking change. If we could confirm it's nonbreaking, we could do it, but not exactly important.

I think it would be breaking to no longer order signals/updates before other non-query-non-patch events. IIRC the only reason I introduced ordering in Python SDK was to get patches and queries in the right place, so I just mimicked TS behavior. But it is set now.

@Sushisource
Copy link
Member Author

This is all quite settled at this point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Rust SDK Issues about or asking for Rust SDK release
Projects
None yet
Development

No branches or pull requests

3 participants