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

fix: Only register signal handlers if user intends to use them #1215

Merged
merged 3 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app.go
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,6 @@ func (app *App) start(ctx context.Context) error {
if err := app.lifecycle.Start(ctx); err != nil {
return err
}
app.receivers.Start(ctx)
return nil
})
}
Expand Down Expand Up @@ -742,6 +741,7 @@ func (app *App) Stop(ctx context.Context) (err error) {
// Alternatively, a signal can be broadcast to all done channels manually by
// using the Shutdown functionality (see the [Shutdowner] documentation for details).
func (app *App) Done() <-chan os.Signal {
app.receivers.Start() // No-op if running
return app.receivers.Done()
}

Expand All @@ -752,6 +752,7 @@ func (app *App) Done() <-chan os.Signal {
// in the [ShutdownSignal] struct.
// Otherwise, the signal that was received will be set.
func (app *App) Wait() <-chan ShutdownSignal {
app.receivers.Start() // No-op if running
return app.receivers.Wait()
}

Expand Down
23 changes: 23 additions & 0 deletions app_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@
package fx

import (
"context"
"errors"
"fmt"
"os"
"sync"
"testing"

Expand Down Expand Up @@ -115,3 +117,24 @@ func TestAnnotationError(t *testing.T) {
assert.ErrorIs(t, err, wantErr)
assert.Contains(t, err.Error(), wantErr.Error())
}

// TestStartDoesNotRegisterSignals verifies that signal.Notify is not called
// when a user starts an app. signal.Notify should only be called when the
// .Wait/.Done are called. Note that app.Run calls .Wait() implicitly.
func TestStartDoesNotRegisterSignals(t *testing.T) {
app := New()
calledNotify := false

// Mock notify function to spy when this is called.
app.receivers.notify = func(c chan<- os.Signal, sig ...os.Signal) {
calledNotify = true
}
app.receivers.stopNotify = func(c chan<- os.Signal) {}

Choose a reason for hiding this comment

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

You should define a variable stopCalled bool and set it to true when called.

And then test its value after calling Stop

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is relevant for this test. #1198 already includes testing that stopNotify gets called when an app stops. I think this is testing that notify does not get called when an app starts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. The only reason I'm setting .stopNotify is because I consider it bad form to call the default signal.Stop func with a channel that was never registered in the first place.

Choose a reason for hiding this comment

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

OK noted


app.Start(context.Background())
defer app.Stop(context.Background())
Copy link

@ccoVeille ccoVeille Jun 24, 2024

Choose a reason for hiding this comment

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

You are not testing it then I think. I mean the test code is not doing what you expect

When defer is called it's too late, no?

Choose a reason for hiding this comment

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

The Stop should be after the wait.

Or you use a t.Cleanup(func() {
app.Stop()
require.True(t, stopCalled)
})

assert.False(t, calledNotify, "notify should not be called when app starts")

_ = app.Wait() // User signals intent have fx listen for signals. This should call notify
assert.True(t, calledNotify, "notify should be called after Wait")
}
26 changes: 25 additions & 1 deletion app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2331,7 +2331,9 @@ func TestHookConstructors(t *testing.T) {
func TestDone(t *testing.T) {
t.Parallel()

done := fxtest.New(t).Done()
app := fxtest.New(t)
defer app.RequireStop()

Choose a reason for hiding this comment

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

In test t.Cleanup is preferred over defer

done := app.Done()
require.NotNil(t, done, "Got a nil channel.")
select {
case sig := <-done:
Expand All @@ -2340,6 +2342,28 @@ func TestDone(t *testing.T) {
}
}

// TestShutdownThenWait tests that if we call .Shutdown before waiting, the wait
// will still return the last shutdown signal.
func TestShutdownThenWait(t *testing.T) {
t.Parallel()

var (
s Shutdowner
)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
app := fxtest.New(
t,
Populate(&s),
)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
defer app.RequireStop()
assert.NotNil(t, s)

err := s.Shutdown(ExitCode(1337))
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)

shutdownSig := <-app.Wait()

Choose a reason for hiding this comment

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

Because of GitHub workflow, I cannot comment the Wait method, so I'm commenting here.

I would expect to use the context as argument

This way Wait could stop when ctx is done.
The same way Stop does.

It's useless in the unit tests, but meaningful in the real app

Choose a reason for hiding this comment

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

I'm unsure but I would expect the same from app.Done

Choose a reason for hiding this comment

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

Last remark, I'm surprised that code in Stop are not closing the channels when leaving on ctx.Done()

require.Equal(t, 1337, shutdownSig.ExitCode)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
}

func TestReplaceLogger(t *testing.T) {
t.Parallel()

Expand Down
1 change: 1 addition & 0 deletions shutdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func TestShutdown(t *testing.T) {
)

require.NoError(t, app.Start(context.Background()), "error starting app")
t.Cleanup(func() { app.Stop(context.Background()) }) // in t.Cleanup so this happens after all subtests return (not just this function)
defer require.NoError(t, app.Stop(context.Background()))

for i := 0; i < 10; i++ {
Expand Down
2 changes: 1 addition & 1 deletion signal.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (recv *signalReceivers) running() bool {
return recv.shutdown != nil && recv.finished != nil
}

func (recv *signalReceivers) Start(ctx context.Context) {
func (recv *signalReceivers) Start() {

Choose a reason for hiding this comment

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

So the context was not used ?

recv.m.Lock()
defer recv.m.Unlock()

Expand Down
10 changes: 4 additions & 6 deletions signal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,7 @@ func TestSignal(t *testing.T) {
t.Parallel()
t.Run("timeout", func(t *testing.T) {
recv := newSignalReceivers()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
recv.Start(ctx)
recv.Start()
timeoutCtx, cancel := context.WithTimeout(context.Background(), 0)
defer cancel()
err := recv.Stop(timeoutCtx)
Expand All @@ -86,8 +84,8 @@ func TestSignal(t *testing.T) {
recv := newSignalReceivers()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
recv.Start(ctx)
recv.Start(ctx) // should be a no-op if already running
recv.Start()
recv.Start() // should be a no-op if already running
require.NoError(t, recv.Stop(ctx))
})
t.Run("notify", func(t *testing.T) {
Expand All @@ -106,7 +104,7 @@ func TestSignal(t *testing.T) {
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
recv.Start(ctx)
recv.Start()
stub <- syscall.SIGTERM
stub <- syscall.SIGTERM
require.Equal(t, syscall.SIGTERM, <-recv.Done())
Expand Down
Loading