From 316385f2d09dc5280ffbb1bb1c8220b6cbb488fa Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Wed, 23 Aug 2023 09:11:51 -0400 Subject: [PATCH] add exit events and embed into eventloop (#21) Signed-off-by: Alex Goodman --- application.go | 23 +++++--- application_test.go | 27 +++++++++ event.go | 21 +++++++ eventloop.go | 28 ++++++++++ eventloop_test.go | 132 +++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 215 insertions(+), 16 deletions(-) create mode 100644 event.go diff --git a/application.go b/application.go index f11fb6d..5ac45d2 100644 --- a/application.go +++ b/application.go @@ -114,7 +114,17 @@ func (a *application) runInitializers() error { func (a *application) WrapRunE(fn func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) error { return func(cmd *cobra.Command, args []string) error { - return a.execute(cmd.Context(), async(cmd, args, fn)) + wrapper := func(cmd *cobra.Command, args []string) error { + defer func() { + // when the worker has completed (or errored) we want to exit the event loop gracefully + if a.state.Bus != nil { + a.state.Bus.Publish(ExitEvent(false)) + } + }() + return fn(cmd, args) + } + + return a.execute(cmd.Context(), async(cmd, args, wrapper)) } } @@ -201,12 +211,9 @@ func (a *application) Run() { panic(fmt.Errorf(setupRootCommandNotCalledError)) } - cmd := a.root - log := a.state.Logger - // drive application control from a single context which can be cancelled (notifying the event loop to stop) ctx, cancel := context.WithCancel(context.Background()) - cmd.SetContext(ctx) + a.root.SetContext(ctx) // note: it is important to always do signal handling from the main package. In this way if quill is used // as a lib a refactor would not need to be done (since anything from the main package cannot be imported this @@ -230,16 +237,16 @@ func (a *application) Run() { go func() { select { case <-signals: // first signal, cancel context - log.Trace("signal interrupt, stop requested") + a.state.Logger.Trace("signal interrupt, stop requested") cancel() case <-ctx.Done(): } <-signals // second signal, hard exit - log.Trace("signal interrupt, killing") + a.state.Logger.Trace("signal interrupt, killing") exitCode = 1 }() - if err := cmd.Execute(); err != nil { + if err := a.root.Execute(); err != nil { color.Red.Println(strings.TrimSpace(err.Error())) exitCode = 1 } diff --git a/application_test.go b/application_test.go index ecee4da..53ff95a 100644 --- a/application_test.go +++ b/application_test.go @@ -353,3 +353,30 @@ func Test_RunExitError(t *testing.T) { } t.Fatalf("process ran with err %v, want exit status 1", err) } + +func Test_Run_InvokesBusExit(t *testing.T) { + runCalled := false + + var sub *partybus.Subscription + + app := New(*NewSetupConfig(Identification{}).WithInitializers( + func(state *State) error { + sub = state.Bus.Subscribe() + return nil + }, + )) + app.SetupRootCommand(&cobra.Command{ + RunE: func(cmd *cobra.Command, args []string) error { + runCalled = true + return nil + }, + }) + app.Run() + require.True(t, runCalled) + + events := sub.Events() + + e := <-events + + assert.Equal(t, e.Type, ExitEventType) +} diff --git a/event.go b/event.go new file mode 100644 index 0000000..c731811 --- /dev/null +++ b/event.go @@ -0,0 +1,21 @@ +package clio + +import ( + "os" + + "github.com/wagoodman/go-partybus" +) + +const ExitEventType partybus.EventType = "clio-exit" + +func ExitEvent(interrupt bool) partybus.Event { + if interrupt { + return partybus.Event{ + Type: ExitEventType, + Value: os.Interrupt, + } + } + return partybus.Event{ + Type: ExitEventType, + } +} diff --git a/eventloop.go b/eventloop.go index 5ddebeb..cfd42d8 100644 --- a/eventloop.go +++ b/eventloop.go @@ -3,6 +3,7 @@ package clio import ( "context" "errors" + "os" "github.com/hashicorp/go-multierror" "github.com/wagoodman/go-partybus" @@ -67,9 +68,36 @@ func eventloop(ctx context.Context, log logger.Logger, subscription *partybus.Su events = nil continue } + + if e.Type == ExitEventType { + events = nil + + if e.Value == os.Interrupt { + // on top of listening to signals from the OS, we also listen to interrupt events from the UI. + // Why provide two ways to do the same thing? In an application that has a UI where the terminal + // has been set to raw mode, ctrl-c will not register as an interrupt signal to the application. + // Instead the UI will capture the ctrl-c and need to signal the event loop to exit gracefully. + // Using the same signal channel for both OS signals and UI signals is not advisable and requires + // injecting the channel into the UI (also not advisable). Providing a bus event for the UI to + // use is a better solution here. + + log.Trace("signal interrupt") + + workerErrs = nil + forceTeardown = true + } else { + log.Trace("signal exit") + } + + if subscription != nil { + _ = subscription.Unsubscribe() + } + } + if ux == nil { continue } + if err := ux.Handle(e); err != nil { if errors.Is(err, partybus.ErrUnsubscribe) { events = nil diff --git a/eventloop_test.go b/eventloop_test.go index cdb4a9b..683a448 100644 --- a/eventloop_test.go +++ b/eventloop_test.go @@ -38,9 +38,9 @@ func (u *uiMock) Handle(event partybus.Event) error { return u.Called(event).Error(0) } -func (u *uiMock) Teardown(_ bool) error { +func (u *uiMock) Teardown(force bool) error { u.t.Logf("UI Teardown called") - return u.Called().Error(0) + return u.Called(force).Error(0) } func Test_EventLoop_gracefulExit(t *testing.T) { @@ -80,7 +80,7 @@ func Test_EventLoop_gracefulExit(t *testing.T) { ux.On("Handle", finalEvent).Return(nil) // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(nil) + ux.On("Teardown", false).Return(nil) assert.NoError(t, eventloop( @@ -131,7 +131,7 @@ func Test_EventLoop_workerError(t *testing.T) { // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(nil) + ux.On("Teardown", true).Return(nil) // ensure we see an error returned assert.ErrorIs(t, @@ -190,7 +190,7 @@ func Test_EventLoop_unsubscribeError(t *testing.T) { ux.On("Handle", finalEvent).Return(partybus.ErrUnsubscribe) // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(nil) + ux.On("Teardown", false).Return(nil) // unsubscribe errors should be handled and ignored, not propagated. We are additionally asserting that // this case is handled as a controlled shutdown (this test should not timeout) @@ -249,7 +249,7 @@ func Test_EventLoop_handlerError(t *testing.T) { ux.On("Handle", finalEvent).Return(finalEvent.Error) // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(nil) + ux.On("Teardown", false).Return(nil) // handle errors SHOULD propagate the event loop. We are additionally asserting that this case is // handled as a controlled shutdown (this test should not timeout) @@ -292,7 +292,7 @@ func Test_EventLoop_contextCancelStopExecution(t *testing.T) { // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(nil) + ux.On("Teardown", true).Return(nil) go cancel() @@ -313,6 +313,122 @@ func Test_EventLoop_contextCancelStopExecution(t *testing.T) { testWithTimeout(t, 5*time.Second, test) } +func Test_EventLoop_ExitEventStopExecution(t *testing.T) { + test := func(t *testing.T) { + + testBus := partybus.NewBus() + subscription := testBus.Subscribe() + t.Cleanup(testBus.Close) + + finalEvent := ExitEvent(false) + + worker := func() <-chan error { + ret := make(chan error) + go func() { + t.Log("worker running") + // send an empty item (which is ignored) ensuring we've entered the select statement, + // then close (a partial shutdown). + ret <- nil + t.Log("worker sent nothing") + close(ret) + t.Log("worker closed") + // do the other half of the shutdown + testBus.Publish(finalEvent) + t.Log("worker published final event") + }() + return ret + } + + ux := &uiMock{ + t: t, + // don't force unsubscribe, allow exit to cause it + } + + // ensure the mock sees at least the final event... note the event error is propagated + ux.On("Handle", finalEvent).Return(nil) + // ensure the mock sees basic setup/teardown events + ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) + ux.On("Teardown", false).Return(nil) + + // handle errors SHOULD propagate the event loop. We are additionally asserting that this case is + // handled as a controlled shutdown (this test should not timeout) + assert.ErrorIs(t, + eventloop( + context.Background(), + discard.New(), + subscription, + worker(), + ux, + ), + finalEvent.Error, + "should have seen a event error, but did not", + ) + + ux.AssertExpectations(t) + } + + // if there is a bug, then there is a risk of the event loop never returning + testWithTimeout(t, 5*time.Second, test) +} + +func Test_EventLoop_InterruptEventKillExecution(t *testing.T) { + test := func(t *testing.T) { + + testBus := partybus.NewBus() + subscription := testBus.Subscribe() + t.Cleanup(testBus.Close) + + finalEvent := ExitEvent(true) + + worker := func() <-chan error { + ret := make(chan error) + go func() { + t.Log("worker running") + // send an empty item (which is ignored) ensuring we've entered the select statement, + // then close (a partial shutdown). + ret <- nil + t.Log("worker sent nothing") + close(ret) + t.Log("worker closed") + // do the other half of the shutdown + testBus.Publish(finalEvent) + t.Log("worker published final event") + }() + return ret + } + + ux := &uiMock{ + t: t, + // don't force unsubscribe, allow exit to cause it + } + + // ensure the mock sees at least the final event... note the event error is propagated + ux.On("Handle", finalEvent).Return(nil) + // ensure the mock sees basic setup/teardown events + ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) + ux.On("Teardown", true).Return(nil) + + // handle errors SHOULD propagate the event loop. We are additionally asserting that this case is + // handled as a controlled shutdown (this test should not timeout) + assert.ErrorIs(t, + eventloop( + context.Background(), + discard.New(), + subscription, + worker(), + ux, + ), + finalEvent.Error, + "should have seen a event error, but did not", + ) + + ux.AssertExpectations(t) + } + + // if there is a bug, then there is a risk of the event loop never returning + testWithTimeout(t, 5*time.Second, test) +} + func Test_EventLoop_uiTeardownError(t *testing.T) { test := func(t *testing.T) { @@ -352,7 +468,7 @@ func Test_EventLoop_uiTeardownError(t *testing.T) { ux.On("Handle", finalEvent).Return(nil) // ensure the mock sees basic setup/teardown events ux.On("Setup", mock.AnythingOfType("func() error")).Return(nil) - ux.On("Teardown").Return(teardownError) + ux.On("Teardown", false).Return(teardownError) // ensure we see an error returned assert.ErrorIs(t,