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

MarcoPolo · 2024-06-17T17:59:04Z

closes #1212, and fixes a regression from #989. Previously we would only register signal handlers if the user intended to use them. #989 changed this behavior here.

This regression meant that if you only used app.Start()/app.Stop(), fx would register signal handlers for no reason as the user didn't use app.Done/app.Wait.

codecov · 2024-06-18T18:00:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.41%. Comparing base (d0d12e9) to head (34bdbf2).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1215   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files          34       34           
  Lines        2908     2909    +1     
=======================================
+ Hits         2862     2863    +1     
  Misses         38       38           
  Partials        8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

app_internal_test.go

app.go

app_internal_test.go

ccoVeille

I found a few things that surprised me. I might be wrong. But I prefer to raise the points

ccoVeille · 2024-06-24T19:35:32Z

app_test.go

@@ -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()


In test t.Cleanup is preferred over defer

ccoVeille · 2024-06-24T19:36:53Z

signal.go

@@ -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() {


So the context was not used ?

ccoVeille · 2024-06-24T19:39:02Z

app_test.go

+	err := s.Shutdown(ExitCode(1337))
+	require.NoError(t, err)
+
+	shutdownSig := <-app.Wait()


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

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

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

ccoVeille · 2024-06-24T19:48:15Z

app_internal_test.go

+	app.receivers.stopNotify = func(c chan<- os.Signal) {}
+
+	app.Start(context.Background())
+	defer app.Stop(context.Background())


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?

The Stop should be after the wait.

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

ccoVeille · 2024-06-24T19:49:52Z

app_internal_test.go

+	app.receivers.notify = func(c chan<- os.Signal, sig ...os.Signal) {
+		calledNotify = true
+	}
+	app.receivers.stopNotify = func(c chan<- os.Signal) {}


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

And then test its value after calling Stop

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.

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.

JacobOaks · 2024-06-25T15:14:25Z

Thanks for updating the PR @MarcoPolo! Mostly LGTM, just a couple small comments.

ccoVeille · 2024-06-25T15:59:05Z

@MarcoPolo or @JacobOaks tell me if the review I made, makes no sense 😅

app_test.go

JacobOaks · 2024-06-25T15:03:51Z

app_internal_test.go

+	app.receivers.notify = func(c chan<- os.Signal, sig ...os.Signal) {
+		calledNotify = true
+	}
+	app.receivers.stopNotify = func(c chan<- os.Signal) {}


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.

Update changelog and `version.go` for a patch release that includes uber-go#1215.

Update changelog and `version.go` for a patch release that includes #1215.

ccoVeille suggested changes Jun 18, 2024

View reviewed changes

app_internal_test.go Outdated Show resolved Hide resolved

app.go Outdated Show resolved Hide resolved

app_internal_test.go Outdated Show resolved Hide resolved

app_internal_test.go Outdated Show resolved Hide resolved

ccoVeille mentioned this pull request Jun 20, 2024

Opt-out of signal handling #1212

Closed

Only register signals if user intends to use them

1fd7e16

MarcoPolo force-pushed the marco/opt-disable-signal-handler branch from 4c557a6 to 1fd7e16 Compare June 24, 2024 17:23

MarcoPolo changed the title ~~feat: Add option to disable signal handler~~ fix: Only register signal handlers if user intends to use them Jun 24, 2024

ccoVeille reviewed Jun 24, 2024

View reviewed changes

Fix msg

23c01a8

JacobOaks reviewed Jun 25, 2024

View reviewed changes

PR comments

34bdbf2

JacobOaks approved these changes Jun 25, 2024

View reviewed changes

JacobOaks merged commit 38e64ec into uber-go:master Jun 25, 2024
12 checks passed

JacobOaks added a commit to JacobOaks/fx that referenced this pull request Jun 25, 2024

Prepare Release v1.22.1

22407c7

Update changelog and `version.go` for a patch release that includes uber-go#1215.

JacobOaks mentioned this pull request Jun 25, 2024

Prepare Release v1.22.1 #1217

Merged

JacobOaks added a commit that referenced this pull request Jun 25, 2024

Prepare Release v1.22.1 (#1217)

45af511

Update changelog and `version.go` for a patch release that includes #1215.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarcoPolo commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

ccoVeille left a comment

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024 •

edited

Loading

ccoVeille Jun 24, 2024

ccoVeille Jun 24, 2024

JacobOaks Jun 25, 2024

MarcoPolo Jun 25, 2024

ccoVeille Jun 25, 2024

JacobOaks commented Jun 25, 2024 •

edited

Loading

ccoVeille commented Jun 25, 2024

JacobOaks Jun 25, 2024

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

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

Conversation

MarcoPolo commented Jun 17, 2024 • edited Loading

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

ccoVeille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobOaks commented Jun 25, 2024 • edited Loading

ccoVeille commented Jun 25, 2024

Choose a reason for hiding this comment

MarcoPolo commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

ccoVeille Jun 24, 2024 •

edited

Loading

JacobOaks commented Jun 25, 2024 •

edited

Loading