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

feat: allow FSMs to specify the next event to send #1723

Merged
merged 8 commits into from
Aug 15, 2024
Merged

Conversation

alecthomas
Copy link
Collaborator

@alecthomas alecthomas commented Jun 10, 2024

closes #1664
closes #2334

This is particularly useful when an FSM state itself knows which state it should be in next, for example transitioning a payment to "voided" if an error occurs communicating with an external vendor.

  • Adds fsm.Next(ctx, event) to the go runtime
    • Only one next transition is allowed per fsm instance
  • When a transition completes, the next transition is queued up if it exists
  • When a transition returns an error, we wipe the next transition so that the retry attempt can set the next transition again

In another PR we will move away from using ftl.Next() to avoid the reference cycle issue.

@ftl-robot ftl-robot mentioned this pull request Jun 10, 2024
@alecthomas alecthomas force-pushed the aat/fsm-set-next-event branch 2 times, most recently from c34d1f8 to 42f0e72 Compare August 9, 2024 00:01
@matt2e matt2e force-pushed the aat/fsm-set-next-event branch 2 times, most recently from e70ecb5 to 3845ebd Compare August 12, 2024 00:28
@matt2e matt2e marked this pull request as ready for review August 14, 2024 03:59
@matt2e matt2e requested a review from a team as a code owner August 14, 2024 03:59
@matt2e matt2e requested review from jonathanj-square and removed request for a team August 14, 2024 03:59
@matt2e
Copy link
Collaborator

matt2e commented Aug 14, 2024

@alecthomas Please review if you get a chance, even though you're not a reviewer because you're the author

Copy link
Collaborator Author

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Ah I can't approve my own PR, but you probably can :)

backend/controller/dal/fsm.go Outdated Show resolved Hide resolved
backend/controller/dal/fsm.go Outdated Show resolved Hide resolved
// Each transition also declares the next state(s) to transition to using State
//
//ftl:retry 2 1s
var fsm = ftl.FSM("fsm",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it be better to introduce ftl.FSMNext() in this PR so we don't establish this pattern at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alecthomas I've added that now. Do you know why the grpc for a call included metadata key value pairs? I can't see it used anywhere, but this seemed like a reasonable use case for it so I piggy backed on it. What do you think?
Isolated commit here: move from fsm.Next() to FSMNext()

}

//ftl:typealias
type EventB Event
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the purpose of the type aliases? Unless there's a reason it would probably be simpler and less confusing to use type EventB struct {}

validateAsyncCall("created", "Rosebud"),
validateAsyncCall("paid", "Rosebud"),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This tests is for encryption, can we move these changes to a new test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to this test are to make sure we are encrypting the payload in the fsm next event table

@matt2e matt2e force-pushed the aat/fsm-set-next-event branch 2 times, most recently from ed4048f to 024bd8f Compare August 15, 2024 00:23
@matt2e matt2e added the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Aug 15, 2024
@matt2e matt2e added this pull request to the merge queue Aug 15, 2024
Merged via the queue into main with commit ea46c80 Aug 15, 2024
70 checks passed
@matt2e matt2e deleted the aat/fsm-set-next-event branch August 15, 2024 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference loop when using fsm.Next() Allow sending events from within an FSM state function
3 participants