Side effect entry can be overtaken if not awaited

[slinkydeveloper]

The SideEffect entry writing can be overtaken if the user doesn't await on the returned promise.

To fix that, we need to internally correctly serialize the operations on the SDK state machine.

[slinkydeveloper]

Reproducer:

var p1 = ctx.sideEffect();
var p2 = ctx.sideEffect();

await p1;
await p2;

Another potential fix is to "reserve/claim journal index", then re-order them in the sdk connection layer before sending them to the runtime.

[slinkydeveloper]

Unassigned to myself for the time being. I opened two PRs with the relative approaches.

[slinkydeveloper]

In both the aforementioned approaches we need to change the signature of ctx.awakeable() to Promise<{id, awakeable_promise}>, because of the side effect retry system. Take the following code as example:

var p1 = ctx.sideEffect(closure, nonZeroRetryPolicy);

var {awakeableId, awakeablePromise} = ctx.awakeable();

var p2 = ctx.sideEffect(closureThatUsesAwakeableId);

await p1;

// what happens next doesn't matter

When invoking ctx.awakeable(), you get back an id that is based on the journal entry index. But because the previous side effect has a non zero retry policy, we can't say for certain what the awakeable entry index is without awaiting p1, hence the given awakeableId used in the second side effect is gonna be wrong.

[slinkydeveloper]

The third approach we also discussed, that would not require changing the signature of ctx.awakeable(), is to do a runtime check of whether the user have awaited the completion of the side effect or not:

var p1 = ctx.sideEffect();
var p2 = ctx.anyMethod(); // throws an error "you must await the side effect"

This is not a great solution though, as it's not type checked and you'll figure out this mistake only at runtime in specific cases...

[slinkydeveloper]

After careful consideration, I start leaning toward the third approach of simply forcing the user to await the side effect before invoking another ctx method. Some thoughts:

Removing side effect retries won't ultimately solve the issue

By removing the side effect retries, we could go around the limitation of changing the signature of ctx.awakeable. This is fine, but it still doesn't prevent a user from creating a function like:

async function doSideEffectAndRetry(ctx: RestateContext) {
  while() {
    await ctx.sideEffect()...
    // if fails
    await ctx.sleep()
  }
}

Then invoking and awaiting it later:

const p1 = doSideEffectAndRetry(ctx);

ctx.awakeable();

await p1; // the awakeable id can be wrong because some side effect/sleep entries might have interleaved it!

So we're back at the same problem.

Concurrency in general is not supported

In principle you can change in the above example ctx.sideEffect() with any other context method, and you still get the same problem, as that concurrently writes the journal. This form of "generic concurrency" is not supported and I don't think it will ever be in the current model. To propose an analogy, this is very similar to spawning another thread in java and invoking the RestateContext both from there and from the service thread.

But some primitive concurrency is supported

What's special about side effects is that in the simpler case of concurrency that we claim to support it's easier to fall in trap, which is how I ended up opening this issue in the first place. The following code is safe:

const p1 = ctx.call(...)

// Do other context stuff

await p1;

While this code is not:

const p1 = ctx.sideEffect(...)

// Do other context stuff

await p1;

The difference lies in the fact that ctx.sideEffect creates 1(+) entries potentially after you await it, because the side effect closure is asynchronous, while every other ctx method creates the entry immediately when you invoke it, even if you don't await it.

The alternatives

• If we want to support the aforementioned primitive concurrency for side effects, either Fix side effects can be overtaken by other entries (reservations approach) #217 or Fix side effects can be overtaken by other entries (executor approach) #218 are fine. But we MUST sacrifice something:
  • Change the signature of ctx.awakeable as described in Side effect entry can be overtaken if not awaited #197 (comment), in order to make sure you get the awakeable id only when the state machine finished to process all the previous side effect retries.
  • Remove side effects retries, as it's unsafe to use it with any other RestateContext method. If a user wants to retry side effects, it can do so but it must be aware that it cannot await "later" as shown in the first code snippet above.
• If we don't want to support the aforementioned primitive concurrency for side effects, then let's just shield the users from mistakenly executing something else before the side effect is awaited. This is hopefully easy to do, as it's just a flag to switch at the beginning and end of the side effect method.