From 0ff0ac84f6eb0e2051daa946cf3ba21b94dba30a Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 26 Nov 2024 18:54:18 -0600 Subject: [PATCH] protofsm: fix race in state machine executor tests In this commit, we fix an existing race in the new `protofsm` state machine executor tests. The race would appear as such: ``` --- FAIL: TestStateMachineMsgMapper (0.00s) state_machine_test.go:165: Error Trace:/home/runner/work/lnd/lnd/protofsm/state_machine_test.go:165 /home/runner/work/lnd/lnd/protofsm/state_machine_test.go:451 Error: Object expected to be of type *protofsm.dummyStateStart, but was *protofsm.dummyStateFin Test: TestStateMachineMsgMapper FAIL FAILgithub.com/lightningnetwork/lnd/protofsm0.116s FAIL ``` This race condition was triggered as before we would start the state machine _then_ register for notifications. In `Start` we emit the starting event, then enter the main loop. If that event gets emitted before our subscription, then we'll miss the event, as the terminal event will be the only one received. We fix this by registering for the events before the daemon has started. --- protofsm/state_machine_test.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/protofsm/state_machine_test.go b/protofsm/state_machine_test.go index 0432f386b7..fc30fcefc3 100644 --- a/protofsm/state_machine_test.go +++ b/protofsm/state_machine_test.go @@ -251,14 +251,14 @@ func TestStateMachineOnInitDaemonEvent(t *testing.T) { // message adapter is called on start up. adapters.On("SendMessages", *pub1, mock.Anything).Return(nil) - stateMachine.Start() - defer stateMachine.Stop() - // As we're triggering internal events, we'll also subscribe to the set // of new states so we can assert as we go. stateSub := stateMachine.RegisterStateEvents() defer stateMachine.RemoveStateSub(stateSub) + stateMachine.Start() + defer stateMachine.Stop() + // Assert that we go from the starting state to the final state. The // state machine should now also be on the final terminal state. expectedStates := []State[dummyEvents, *dummyEnv]{ @@ -292,14 +292,15 @@ func TestStateMachineInternalEvents(t *testing.T) { InitEvent: fn.None[DaemonEvent](), } stateMachine := NewStateMachine(cfg) - stateMachine.Start() - defer stateMachine.Stop() // As we're triggering internal events, we'll also subscribe to the set // of new states so we can assert as we go. stateSub := stateMachine.RegisterStateEvents() defer stateMachine.RemoveStateSub(stateSub) + stateMachine.Start() + defer stateMachine.Stop() + // For this transition, we'll send in the emitInternal event, which'll // send us back to the starting event, but emit an internal event. stateMachine.SendEvent(&emitInternal{}) @@ -343,14 +344,15 @@ func TestStateMachineDaemonEvents(t *testing.T) { InitEvent: fn.None[DaemonEvent](), } stateMachine := NewStateMachine(cfg) - stateMachine.Start() - defer stateMachine.Stop() // As we're triggering internal events, we'll also subscribe to the set // of new states so we can assert as we go. stateSub := stateMachine.RegisterStateEvents() defer stateMachine.RemoveStateSub(stateSub) + stateMachine.Start() + defer stateMachine.Stop() + // As soon as we send in the daemon event, we expect the // disable+broadcast events to be processed, as they are unconditional. adapters.On( @@ -428,14 +430,17 @@ func TestStateMachineMsgMapper(t *testing.T) { MsgMapper: fn.Some[MsgMapper[dummyEvents]](dummyMapper), } stateMachine := NewStateMachine(cfg) - stateMachine.Start() - defer stateMachine.Stop() // As we're triggering internal events, we'll also subscribe to the set // of new states so we can assert as we go. + // + // We register before calling Start to ensure we don't miss any events. stateSub := stateMachine.RegisterStateEvents() defer stateMachine.RemoveStateSub(stateSub) + stateMachine.Start() + defer stateMachine.Stop() + // First, we'll verify that the CanHandle method works as expected. require.True(t, stateMachine.CanHandle(wireError)) require.False(t, stateMachine.CanHandle(&lnwire.Init{}))