Repository: uber-go/fx
Pull request #875 (2022-04-20 20:36:13)

Do not run lifecycle hooks with expired context
Currently, the lifecycle hooks are still run even when the context
passed as parameter to Start/Stop calls has expired/DeadlineExceeded.

This changes the fx lifecycle hook execution loop to check for expired
context before executing each hook, and bailing out of the exec loop
upon encountering context expiration.

Refs GO-1026.

codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) Report
> Merging [#875](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (5bb0d12) into [master](https://codecov.io/gh/uber-go/fx/commit/7e721048dcb829028c9e3c540413b002768f10bd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (7e72104) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master     #875   +/-   ##
=======================================
  Coverage   98.52%   98.53%           
=======================================
  Files          30       30           
  Lines        1153     1157    +4     
=======================================
+ Hits         1136     1140    +4     
  Misses         11       11           
  Partials        6        6           
```


| [Impacted Files](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [internal/lifecycle/lifecycle.go](https://codecov.io/gh/uber-go/fx/pull/875/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-aW50ZXJuYWwvbGlmZWN5Y2xlL2xpZmVjeWNsZS5nbw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Last update [7e72104...5bb0d12](https://codecov.io/gh/uber-go/fx/pull/875?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).

sywhang: > Is there a reason to do the allowlist of errors instead of just all of them?

Oh, I missed this comment. That's fair. I'll just check for non-nil errors from the context. 

Patch:
diff --git a/internal/lifecycle/lifecycle.go b/internal/lifecycle/lifecycle.go
index 6916333..af458fa 100644
--- a/internal/lifecycle/lifecycle.go
+++ b/internal/lifecycle/lifecycle.go
@@ -77,6 +77,11 @@ func (l *Lifecycle) Start(ctx context.Context) error {
 	l.mu.Unlock()
 
 	for _, hook := range l.hooks {
+		// if ctx has cancelled, bail out of the loop.
+		if err := ctx.Err(); err != nil {
+			return err
+		}
+
 		if hook.OnStart != nil {
 			l.mu.Lock()
 			l.runningHook = hook
@@ -131,6 +136,9 @@ func (l *Lifecycle) Stop(ctx context.Context) error {
 	// Run backward from last successful OnStart.
 	var errs []error
 	for ; l.numStarted > 0; l.numStarted-- {
+		if err := ctx.Err(); err != nil {
+			return err
+		}
 		hook := l.hooks[l.numStarted-1]
 		if hook.OnStop == nil {
 			continue


Test patch:
diff --git a/internal/lifecycle/lifecycle_test.go b/internal/lifecycle/lifecycle_test.go
index f325de5..4fe75e2 100644
--- a/internal/lifecycle/lifecycle_test.go
+++ b/internal/lifecycle/lifecycle_test.go
@@ -119,6 +119,30 @@ func TestLifecycleStart(t *testing.T) {
 		assert.Equal(t, 2, starterCount, "expected the first and second starter to execute")
 		assert.Equal(t, 1, stopperCount, "expected the first stopper to execute since the second starter failed")
 	})
+
+	t.Run("DoNotRunStartHooksWithExpiredCtx", func(t *testing.T) {
+		t.Parallel()
+
+		l := New(testLogger(t), fxclock.System)
+		l.Append(Hook{
+			OnStart: func(context.Context) error {
+				assert.Fail(t, "this hook should not run")
+				return nil
+			},
+			OnStop: func(context.Context) error {
+				assert.Fail(t, "this hook should not run")
+				return nil
+			},
+		})
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel()
+		err := l.Start(ctx)
+		require.Error(t, err)
+		// Note: Stop does not return an error here because no hooks
+		// have been started, so we don't end up any of the corresponding
+		// stop hooks.
+		require.NoError(t, l.Stop(ctx))
+	})
 }
 
 func TestLifecycleStop(t *testing.T) {
@@ -247,6 +271,26 @@ func TestLifecycleStop(t *testing.T) {
 		assert.Equal(t, err, l.Start(context.Background()))
 		l.Stop(context.Background())
 	})
+
+	t.Run("DoNotRunStopHooksWithExpiredCtx", func(t *testing.T) {
+		t.Parallel()
+
+		l := New(testLogger(t), fxclock.System)
+		l.Append(Hook{
+			OnStart: func(context.Context) error {
+				return nil
+			},
+			OnStop: func(context.Context) error {
+				assert.Fail(t, "this hook should not run")
+				return nil
+			},
+		})
+		ctx, cancel := context.WithCancel(context.Background())
+		err := l.Start(ctx)
+		require.NoError(t, err)
+		cancel()
+		require.Error(t, l.Stop(ctx))
+	})
 }
 
 func TestHookRecordsFormat(t *testing.T) {