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 nil panic on lifecycle OnStart/OnStop #917

Merged
merged 1 commit into from Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [1.18.1] = 2022-08-08
### Fixed
- Fix a nil panic when `nil` is passed to `OnStart` and `OnStop` lifecycle methods.

## [1.18.0] - 2022-08-05
### Added
- Soft value groups that lets you specify value groups as best-effort dependencies.
Expand Down
9 changes: 9 additions & 0 deletions internal/lifecycle/lifecycle.go
Expand Up @@ -22,6 +22,7 @@ package lifecycle

import (
"context"
"errors"
"fmt"
"io"
"strings"
Expand Down Expand Up @@ -72,6 +73,10 @@ func (l *Lifecycle) Append(hook Hook) {
// Start runs all OnStart hooks, returning immediately if it encounters an
// error.
func (l *Lifecycle) Start(ctx context.Context) error {
if ctx == nil {
return errors.New("called OnStart with nil context")
}

l.mu.Lock()
l.startRecords = make(HookRecords, 0, len(l.hooks))
l.mu.Unlock()
Expand Down Expand Up @@ -129,6 +134,10 @@ func (l *Lifecycle) runStartHook(ctx context.Context, hook Hook) (runtime time.D
// Stop runs any OnStop hooks whose OnStart counterpart succeeded. OnStop
// hooks run in reverse order.
func (l *Lifecycle) Stop(ctx context.Context) error {
if ctx == nil {
return errors.New("called OnStop with nil context")
}

l.mu.Lock()
l.stopRecords = make(HookRecords, 0, l.numStarted)
l.mu.Unlock()
Expand Down
26 changes: 26 additions & 0 deletions internal/lifecycle/lifecycle_test.go
Expand Up @@ -291,6 +291,32 @@ func TestLifecycleStop(t *testing.T) {
cancel()
require.Error(t, l.Stop(ctx))
})

t.Run("nil ctx", 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
},
})
//lint:ignore SA1012 this test specifically tests for the lint failure
err := l.Start(nil)
require.Error(t, err)
assert.Contains(t, err.Error(), "called OnStart with nil context")

//lint:ignore SA1012 this test specifically tests for the lint failure
err = l.Stop(nil)
require.Error(t, err)
assert.Contains(t, err.Error(), "called OnStop with nil context")

})
}

func TestHookRecordsFormat(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion version.go
Expand Up @@ -21,4 +21,4 @@
package fx

// Version is exported for runtime compatibility checks.
const Version = "1.18.0"
const Version = "1.18.1"