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

command persistence now will purge NextAwaiter state store #92

Merged

Conversation

Kelerchian
Copy link
Contributor

No description provided.

@Kelerchian Kelerchian linked an issue Jan 25, 2024 that may be closed by this pull request
@Kelerchian Kelerchian requested a review from rkuhn January 25, 2024 14:27
@@ -520,10 +527,12 @@ export const createMachineRunnerInternal = <

emitter.on('next', nva.push)
emitter.on('failure', nva.fail)
emitter.on('commandPersisted', nva.purge)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this might have been a misunderstanding or I might miss something: my understanding was that

someState.commands().someCommand()

should directly purge the awaiter state; I didn’t think that we need to wait until persistence is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought that too, but then I thought purge should happen only when an event has been published.
When the swarm protocol is linear, this event publishing will be a key to the continuation of it;
there is a higher guarantee that a next-triggering state change will happen, either triggered by the published event in question or a peer reacting to said event.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that’s a good point, thanks for the explanation!

Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

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

Looking good, apart from the Promise ordering thingy.

@@ -520,10 +527,12 @@ export const createMachineRunnerInternal = <

emitter.on('next', nva.push)
emitter.on('failure', nva.fail)
emitter.on('commandPersisted', nva.purge)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, that’s a good point, thanks for the explanation!

let nextOrPeekTriggered = false
const peek = machine.peekNext()
const next = machine.next()
Promise.race([peek, next]).finally(() => (nextOrPeekTriggered = true))
Copy link
Member

Choose a reason for hiding this comment

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

same as we discussed yesterday, which is probably in many other tests as well: after this line we need

await Promise.resolve()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. await r1.eventRoundtrip.waitAllDone() was below this line; I moved it up, forgetting that it is important down there too.

@Kelerchian Kelerchian force-pushed the ada/90-command-call-consumes-nextawaiterstate-state-store branch 2 times, most recently from 6e8fd26 to 1d23910 Compare January 25, 2024 16:37
@Kelerchian Kelerchian force-pushed the ada/90-command-call-consumes-nextawaiterstate-state-store branch from 1d23910 to 785f4db Compare January 25, 2024 16:41
@Kelerchian Kelerchian merged commit 7c34a27 into master Jan 25, 2024
1 check passed
@Kelerchian Kelerchian deleted the ada/90-command-call-consumes-nextawaiterstate-state-store branch January 25, 2024 16:58
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.

Command call consumes NextAwaiterState state store
2 participants