Deterministic promise combinators

[slinkydeveloper]

Fix #15

Feature scope

To gradually implement this feature, I reduced the scope of this PR as follows:

• The "combinators" API is exposed through the RestateContext: ctx.all, ctx.race, ctx.any and ctx.allSettled. There was a discussion here Determinism of Promise.any/all #15 (comment) about overriding the globals, but for the time being the methods in the context will do fine.
• The "combinators" API will accept only some promises marked as "combineable".
• Only sleep and awakeable returned promises are "combineable". In particular, I left out:
  • call handler API: this should be straightforward to do. I expect to fix this in a follow up PR.
  • call gRPC API: this is a bit more complicated, as we cannot control the generated code output. This is perhaps related to Consider replacing protobuf library #209, where we discussed about replacing the protobuf library and build our own protoc plugin. I think we should fix this once we settle Consider replacing protobuf library #209.
  • sideEffect: this is a bit more complicated to support because of the side effect retry system, which might cause races in writing entries out. I don't plan to fix this for the time being, a user can just combine promises inside the side effect closure as they normally would do.
  • get: this doesn't seem useful to put in a combinator.
  • Non Restate promises. These will be never supported with this feature design, as we rely on being able to index promises to deterministically replay them.
• No nested combinators are supported. This is something I expect to fix in a followup PR, as it seems a useful feature, e.g. imagine the combination all P1 and P2 where P1 = any(A1, sleep) and P2 = any(A2, sleep). Although priority wise, figuring out call seems more important.

Implementation details

Given the feature scope, the idea is the following: Every time the user awaits a combinator, we record the order of which we see completions for the child promises. E.g. for ALL(P1, P2, P3) we record the order we saw the completions, e.g. C3, C1, C2. Once the combinator is completed (either fullfilled or rejected), we write in the journal what was the order in which we saw the completions. On replay, we make sure the combinator deterministically replays the order in which the completions were seen.

The business logic that records the completion order and ensures on replay the order is replayed deterministically is in the class called PromiseCompletionTracker.
This class is then wired up to the state machine to read/write the CombinatorEntryMessage, that is the entry used to record the completions order, and the RestateContext APIs, to allow users to create those combinators.

[slinkydeveloper]

TODO for me:

• Add a test in this PR with the case where I create the combinator first, I do some other ctx writes, and then I await the combinator. This should create the combinator entry on the await point, and not before.
• E2E tests.

[StephanEwen]

Generally, +1 to this approach, with two main thoughts.

Names

Should we call it RestatePromise rather than CombinablePromise?
Putting myself in the shoes of a user, if I see RestatePromise I have an idea on how to place this (it's a promise from Restate, the system for durable promises - kind of makes sense). For CombinablePromise I don't see that immediately. Might be some node internal or library promise type?

API

The methods for all(), any(), etc. don't really belong to the context for me. The fact that those operations require to store some data in Restate is more of an implementation detail that should not leak into the API.

Could we do the following?

• define static methods RestatePromise.all(), etc.
• have the context in the CombinablePromise (or RestatePromise) as __restatectx. This would act as the tag (which currently is the __combinable property).
• The static method would check that both promises indeed have that property (not just rely on compile time type checks) and that both are the same object (===) and then use that context to create the journal entry.

[StephanEwen]

Did this sneak in as a leftover from some other change?

[slinkydeveloper]

No, this method was simply moved in a more logical place (close to the other call methods)

[StephanEwen]

I don't fully get the logic that tags the CombinablePromise. Maybe I am overlooking something, but it looks a bit incomplete still. Here are the core points:

• The promise has the __combinable property as a tag. But that property isn't checked anywhere. Looks like the code relies purely on type safety from the TS compiler typing.

• While this probably catches most cases during development, none of this info exists at runtime, so double checking whether you actually get a CombinablePromise by checking that property makes sense and is good practice, as I understand it.

• The __combinable property is set as a property that takes the value undefined. This is a really special way of doing it, which means that promise.__combinable === undefined no matter whether something is a CombinablePromise or not. Just that promise.hasOwnProperty("__combinable") would be false for non-CombinablePromise types.

  Why not just make it a boolean flag, and you can directly check where ever you expect a CombinablePromise whether promise.__combinable === true?

  This would also simplify the setter code (promise as any).__combinable = true; or do const cp = promise as CombinablePromise; cp.__combinable = true; return cp;

[StephanEwen]

Do the promises change the suspension behavior?

[slinkydeveloper]

No they don't, this was a little cleanup i did while analyzing this code.

[slinkydeveloper]

Should we call it RestatePromise rather than CombinablePromise?

I gave some thoughts to it. My problem with calling it just RestatePromise is that it will prompt myself the question of "why then side effect returns a regular Promise? what's special about RestatePromise?"

CombineablePromise on the other hand clearly defines an additional supported behavior on the promise. Does that make somewhat sense?

[slinkydeveloper]

@StephanEwen I updated this PR changing how we expose the combinator methods, doing as you suggested: CombineablePromise.all(), etc. Can you give it another look now?