FSM: add ctx to SendEvent and Actions #839
Conversation
sputn1ck
force-pushed
the
fsm_ctx
branch
2 times, most recently
from
24fbd52 to
f9ff93b
Compare
hieblmi
left a comment
hieblmi
left a comment
There was a problem hiding this comment.
I think we can remove the m.runCtx from the reservations manager. Otherwise just nits.
|
|
||
| // Action represents the action to be executed in a given state. | ||
| type Action func(eventCtx EventContext) EventType | ||
| type Action func(ctx context.Context, eventCtx EventContext) EventType |
There was a problem hiding this comment.
Currently the EventContext is static across a SendEvent loop. I think it would be nice to have a way to communicate data from one action to the next, which would require something like
type Action func(ctx context.Context, eventCtx EventContext) (EventType, EventContext)
If you think that's a good idea maybe we can create an issue and add that in a follow-up PR?
There was a problem hiding this comment.
I'm not sure if this is really needed, as now every action explicitely needs to have the eventCtx as well as return it, e.g. prior. func DoStuff(_ context.Context, _ EventContext) would now always need to set the EventContext as well as return it in every path. I think either storing data in the respective FSM struct or mutating the passed eventCtx should be enough.
There was a problem hiding this comment.
Okay tested it a bit, mutating the type of the EventContext in an action does apparently not work
There was a problem hiding this comment.
Maybe another working approach would be to have the initial EventContext have the fields already for the upcoming actions. E.g.
type StuffEventCtx struct{
initialReq string
respondChan chan string
dataFromAction1 string
}
but might also not be ideal
There was a problem hiding this comment.
Instead of mutating why not simply return the context as suggested by @hieblmi ? Otherwise I agree this should be a separate follow up PR.
There was a problem hiding this comment.
If mutating we could pass the context to SendEvent and ActionEntry as a reference and alter it. But having all folks modifying a single reference might also cause issues.
| err := f.cfg.Store.UpdateInstantLoopOut(f.ctx, f.InstantOut) | ||
| err := f.cfg.Store.UpdateInstantLoopOut(ctx, f.InstantOut) | ||
| if err != nil { | ||
| log.Errorf("Error updating instant out: %v", err) |
There was a problem hiding this comment.
We don't usually add newlines in error returns right?
This commit updates the reservation package to use the new fsm context instead of the old fsm context. This is only a first step in the process of migrating the reservation package to the new fsm context. The next step should be to remove the stored context in the reservation manager.
This commit updates the instantout package to use the new fsm context instead of the old fsm context. This is only a first step in the process of migrating the instantout package to the new fsm context. The next step should be to remove the stored context in the instantout manager.
This fixes a flake in the unit tests
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
LGTM, nice to get rid of the embedded context 🎉
|
|
||
| // Action represents the action to be executed in a given state. | ||
| type Action func(eventCtx EventContext) EventType | ||
| type Action func(ctx context.Context, eventCtx EventContext) EventType |
There was a problem hiding this comment.
Instead of mutating why not simply return the context as suggested by @hieblmi ? Otherwise I agree this should be a separate follow up PR.
3 participants
This PR adds a ctx parameter to the
SendEventFunctions and all interfaces.