-
-
Notifications
You must be signed in to change notification settings - Fork 158
RFC: Add a state machine implementation #218
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
base: v310
Are you sure you want to change the base?
Conversation
Hi Jeroen, Thanks for the great contribution — I really appreciate it! That said, I’m still a bit uncertain about the design of the FSM implementation. My first attempt was actually quite similar to your approach, using reflection. However, in the end, I found it wasn’t as clean and elegant as I’d hoped. I'll take a deeper look into your code - maybe we can find an even better solution together. |
Thanks Taras. I copied an example of what the consumer API looks like in the PR description as well. I'm not a big fan of the reflection code either, but I couldn't find another way, especially whilst keeping the API of the FSM reasonably elegant and type-safe. That being said, I am relatively new to Go, so maybe there are better ways of doing this. Let me know! Once we agree on an approach, I would be happy to expand on this PR and add some of the additional functionality available in |
c63516a
to
039be32
Compare
Hi @halturin, I have cleaned up this PR to make it a bit more readable. I've also changed the API to be more similar to the other actors (replacing Let me know what you think and if you have any specific concerns you want me to look into. Happy to have a more synchronous conversation about this as well :) PS: the discord link on GH is invalid for me? |
ed02bf2
to
61a4fb6
Compare
agree with it. no need to couple.
glad to see how you aligning with the framework. the only think, atm, not sure about the functional approach PS: great progress 👍 |
cc7c3f5
to
c428942
Compare
Is it the naming or the functional options pattern in general? If it is the naming, we can remove the PS: I just pushed support for state timeouts. State enter callback support was already added. I will add generic timeouts and event timeouts next and update the example to demonstrate the newly added features. I might just port the lock example from the gen_statem documentation. |
092f4fa
to
ccfdc65
Compare
this :) not the naming. just to align with the common style. what do you think about using an event as a trigger for state transitions without requiring a callback function? PS: i’ve also noticed a panic when Monitor events fail. perhaps we should retry until it succeeds? not sure which approach is best yet — just an idea... |
forgot to mention… let's create a separate example in ergo.services/examples for the StateMachine, with a clearer and more visual demonstration of its functionality I can also give you access to docs.ergo.services for writing documentation |
Not sure how this would work with an event trigger, could you elaborate more on this approach? One other approach I was thinking of, sacrificing type safety for consistency: we could allow for registering a single handler per state instead of per state and message type. This is actually what Erlang does as well. Of course, Erlang has pattern matching, so you're still effectively dispatching on the type of the message. In this approach, we would deliver any incoming message to the handler for the state the state machine is currently in. Then it is up to the user to do the type assertions on the incoming message (now of type I want to clean up the current branch so that this is finished. We could try the alternate approach on a separate branch. I'm not 100% sure yet this would work, I'd have to see the code first. Take a look at the updated example with the existing API and let me know what you think. One other thought: we could also align the rest of ergo with this PR. It all comes down to whether we want to dispatch messages based on their type or if a single catch-all handler is preferred, and it is up to the user to interrogate the type with assertions and dispatch to the correct logic themselves.
Yea unsure about this. If I register a handler for an event, do I expect that event to be registered or not? The semantics become a bit fuzzy if the event cannot be registered. What if I made a typo in the event, how will I find out? What will be the retry strategy for the actor to call the PS: docs access would be good, I want to finalize the API first though before writing those |
79498f9
to
2a1b572
Compare
The following code compiles, it doesn't actually work yet, but should give a good idea what the handler per state solution would look like:
FWIW, with this approach, I don't think we need to use any reflection anymore |
@halturin did you get a chance to look at these approaches? what do you think? |
Not yet, sorry. I've had a tough time over the last couple of weeks. This week, I hope. PS: I sent an invitation to the gitbook. Pls, add your page in the Actors section. |
may I ask you to change the target to v310 branch. I'm thinking of including it as a part of the 3.1 release |
2a1b572
to
c9b2592
Compare
Just rebased v310 and updated the base branch. Have you given the 2 different approaches some thought? I will write up the docs once we decided which one we'll go with. Summarizing my thoughts about the 2 approaches: Approach 1 (the current implementation) Pros:
Cons:
Approach 2 (as suggested in the comment) Pros:
Cons:
I do think if we were to go with approach 1, that it would be worth considering whether (at some point in the future) the other actors could follow a similar pattern, meaning in
|
I’ll take a closer look at your PR tomorrow (or the day after). As for the two approaches... The main issue with generics is that they significantly impact performance, so I'm not sure the second one would work well for the high load scenario (in terms of performance, I mean). UPD: forgot to mention - that’s actually the reason I use |
I’ve reviewed your implementation carefully — you’ve done a great and impressive job!
Overall, it's a solid implementation. We just need to think a bit more about how to avoid relying on reflection for method dispatch |
In the best scenario, using generic costs you around 30% of performance https://planetscale.com/blog/generics-can-make-your-go-code-slower |
Thanks Taras!
Agree on moving this over. Could you give me access to this repo?
I initially wanted to do this, but if I use
I believe the only place where this is happening is in the timeouts. It is working, at least, the tests are passing? Are there any concerns with the current implementation.
Agreed that the reflection code is suboptimal. By far the easiest way around this is to use the alternative approach I suggested in the comments. I think by keeping the functional options pattern, we can still have decent type safety (the same level of type safety as users get throughout the rest of the framework). By not making What do you think? |
Hi @halturin, friendly nudge here to check what direction you want me to take with this. |
RFC: Add a state machine implementation
Background
This draft PR proposes a
StateMachine
to be added to the standard actors library. The implementation is loosely based on gen_statem. It operates similarly togen_statem
in callback modestate_functions
, but instead of a single callback per state we use a callback per(state, messageType)
tuple.Implementation
The
StateMachine
is implemented as an Actor in the standard actors library. It follows the same pattern as theSupervisor
where theInit
function implementation is mandatory and returns the specification of theStateMachine
. The specification in this draft PR contains the initial state as well as the callback functions for the states. States are of typegen.Atom
, similar to their Erlang/Elixir counterparts, and theStateMachine
has a generic type parameterD
for the data associated with the state machine (again akin to the Erlang implementation). A difference with the Erlang implementation is that the data and the new state are not returned from the state callback functions, instead they are part of the mutable state of the actor.For creating the
StateMachineSpec
there are several helper functions and types as we need to capture the type of the message being processed by the callbacks. We also want to reduce unnecessary copying of the spec, therefore we use the functional option pattern.When attempting an invalid state transition, following Erlang/OTP's "let it crash" philosophy, the process terminates.
API
Below is an example of what the API for the consumer of the state machine looks like. There is a [PR (https://github.com/ergo-services/examples/pull/8) in the examples repo as well
Open questions
EventContent
is delivered to timeout callbacksTODO