Skip to content

Refactor WatchConfig to panic inline instead of os.Exit() in a goroutine on init fail (#2095) #2096

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.

Sign up for GitHub

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

Open
deefdragon wants to merge 3 commits into
base: master
Choose a base branch
from
+57 −62
Open

Refactor WatchConfig to panic inline instead of os.Exit() in a goroutine on init fail (#2095) #2096

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1b8f81
Refactor WatchConfig to panic inline instead of os.Exit() in a gorout…
deefdragon Nov 21, 2025
ed1917e
Make errors consistent between the logged error and the error in the …
deefdragon Nov 22, 2025
f86aae5
Update comment for WatchConfig to denote it may panic
deefdragon Nov 22, 2025
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
119 changes: 57 additions & 62 deletions viper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"slices"
"strconv"
"strings"
"sync"
"time"

"github.com/fsnotify/fsnotify"
Expand Down Expand Up @@ -276,80 +275,76 @@ func (v *Viper) OnConfigChange(run func(in fsnotify.Event)) {
}

// WatchConfig starts watching a config file for changes.
// If there is an error watching the config file, WatchConfig will panic.
func WatchConfig() { v.WatchConfig() }

// WatchConfig starts watching a config file for changes.
// If there is an error watching the config file, WatchConfig will panic.
Comment on lines +278 to +282
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

This global WatchConfig comment says the function will panic on any error "watching" the config file, but only setup errors panic; runtime watch/read errors are logged. Consider rewording to explicitly describe panic-on-setup-failure behavior.

Suggested change
// If there is an error watching the config file, WatchConfig will panic.
func WatchConfig() { v.WatchConfig() }
// WatchConfig starts watching a config file for changes.
// If there is an error watching the config file, WatchConfig will panic.
// It panics if setting up the watcher fails (for example, when creating the watcher,
// resolving the config file, or adding the config directory); subsequent watch or read
// errors are logged.
func WatchConfig() { v.WatchConfig() }
// WatchConfig starts watching a config file for changes.
// It panics if setting up the watcher fails (for example, when creating the watcher,
// resolving the config file, or adding the config directory); subsequent watch or read
// errors are logged.

Copilot uses AI. Check for mistakes.
Comment on lines +278 to +282
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The doc comment says WatchConfig will panic "if there is an error watching the config file", but the implementation only panics on setup failures (NewWatcher/getConfigFile/Add). Runtime watcher errors and ReadInConfig errors are only logged. Please adjust the wording to match the actual behavior (e.g., panic on initialization/setup failure).

Suggested change
// If there is an error watching the config file, WatchConfig will panic.
func WatchConfig() { v.WatchConfig() }
// WatchConfig starts watching a config file for changes.
// If there is an error watching the config file, WatchConfig will panic.
// WatchConfig panics if it fails to initialize watching the config file (for example, creating the watcher or adding the config directory).
func WatchConfig() { v.WatchConfig() }
// WatchConfig starts watching a config file for changes.
// WatchConfig panics if it fails to initialize watching the config file (for example, creating the watcher or adding the config directory).

Copilot uses AI. Check for mistakes.
func (v *Viper) WatchConfig() {
initWG := sync.WaitGroup{}
initWG.Add(1)

watcher, err := fsnotify.NewWatcher()
if err != nil {
err = fmt.Errorf("failed to create watcher: %s", err)
v.logger.Error(err.Error())
panic(err)
Comment on lines +285 to +289
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

Use error wrapping with %w (instead of %s) when building the panic error so callers can inspect the root cause via errors.Is/As. This repo already uses %w in similar logger.Error(fmt.Errorf(...).Error()) patterns.

Copilot uses AI. Check for mistakes.
}
// we have to watch the entire directory to pick up renames/atomic saves in a cross-platform way
filename, err := v.getConfigFile()
if err != nil {
err = fmt.Errorf("failed to get config file: %s", err)
v.logger.Error(err.Error())
Comment on lines +292 to +295
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

These new panic-on-init-failure branches are not covered by existing WatchConfig tests. Please add a test that asserts WatchConfig panics on setup failure (e.g., getConfigFile error or watcher.Add error) so this behavior remains stable across refactors.

Copilot uses AI. Check for mistakes.
Comment on lines +294 to +295
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

Use %w instead of %s in fmt.Errorf here so the returned/panicked error wraps the underlying cause for errors.Is/As checks.

Copilot uses AI. Check for mistakes.
watcher.Close()
panic(err)
}

configFile := filepath.Clean(filename)
configDir, _ := filepath.Split(configFile)
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

filepath.Split can yield an empty configDir when the config file path has no directory component (e.g. "config.yaml"), which makes watcher.Add("") fail and now panic. Consider using filepath.Dir and normalizing empty to "." (or otherwise ensuring a valid directory path) before calling watcher.Add.

Suggested change
configDir, _ := filepath.Split(configFile)
configDir := filepath.Dir(configFile)
if configDir == "" {
configDir = "."
}

Copilot uses AI. Check for mistakes.
realConfigFile, _ := filepath.EvalSymlinks(filename)

err = watcher.Add(configDir)
if err != nil {
err = fmt.Errorf("failed to add config dir to watcher: %s", err)
v.logger.Error(err.Error())
Comment on lines +306 to +307
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

Use %w instead of %s in fmt.Errorf here so the panicked error preserves the underlying failure for errors.Is/As checks.

Copilot uses AI. Check for mistakes.
watcher.Close()
panic(err)
}

go func() {
watcher, err := fsnotify.NewWatcher()
if err != nil {
v.logger.Error(fmt.Sprintf("failed to create watcher: %s", err))
os.Exit(1)
}
defer watcher.Close()
// we have to watch the entire directory to pick up renames/atomic saves in a cross-platform way
filename, err := v.getConfigFile()
if err != nil {
v.logger.Error(fmt.Sprintf("get config file: %s", err))
initWG.Done()
return
}

configFile := filepath.Clean(filename)
configDir, _ := filepath.Split(configFile)
realConfigFile, _ := filepath.EvalSymlinks(filename)

eventsWG := sync.WaitGroup{}
eventsWG.Add(1)
go func() {
for {
select {
case event, ok := <-watcher.Events:
if !ok { // 'Events' channel is closed
eventsWG.Done()
return
for {
select {
case event, ok := <-watcher.Events:
if !ok { // 'Events' channel is closed
return
}
currentConfigFile, _ := filepath.EvalSymlinks(filename)
// we only care about the config file with the following cases:
// 1 - if the config file was modified or created
// 2 - if the real path to the config file changed (eg: k8s ConfigMap replacement)
if (filepath.Clean(event.Name) == configFile &&
(event.Has(fsnotify.Write) || event.Has(fsnotify.Create))) ||
(currentConfigFile != "" && currentConfigFile != realConfigFile) {
realConfigFile = currentConfigFile
err := v.ReadInConfig()
if err != nil {
v.logger.Error(fmt.Sprintf("read config file: %s", err))
}
currentConfigFile, _ := filepath.EvalSymlinks(filename)
// we only care about the config file with the following cases:
// 1 - if the config file was modified or created
// 2 - if the real path to the config file changed (eg: k8s ConfigMap replacement)
if (filepath.Clean(event.Name) == configFile &&
(event.Has(fsnotify.Write) || event.Has(fsnotify.Create))) ||
(currentConfigFile != "" && currentConfigFile != realConfigFile) {
realConfigFile = currentConfigFile
err := v.ReadInConfig()
if err != nil {
v.logger.Error(fmt.Sprintf("read config file: %s", err))
}
if v.onConfigChange != nil {
v.onConfigChange(event)
}
} else if filepath.Clean(event.Name) == configFile && event.Has(fsnotify.Remove) {
eventsWG.Done()
return
if v.onConfigChange != nil {
v.onConfigChange(event)
}

case err, ok := <-watcher.Errors:
if ok { // 'Errors' channel is not closed
v.logger.Error(fmt.Sprintf("watcher error: %s", err))
}
eventsWG.Done()
} else if filepath.Clean(event.Name) == configFile && event.Has(fsnotify.Remove) {
return
}

case err, ok := <-watcher.Errors:
if ok { // 'Errors' channel is not closed
v.logger.Error(fmt.Sprintf("watcher error: %s", err))
}
return
}
}()
err = watcher.Add(configDir)
if err != nil {
v.logger.Error(fmt.Sprintf("failed to add watcher: %s", err))
initWG.Done()
return
}
initWG.Done() // done initializing the watch in this go routine, so the parent routine can move on...
eventsWG.Wait() // now, wait for event loop to end in this go-routine...
}()
initWG.Wait() // make sure that the go routine above fully ended before returning
Comment on lines -350 to -352
Copy link
Contributor

@ccoVeille ccoVeille Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm surprised by the changes about wait group here, and all the changes you did in the file to do not use waitgroup

I would have kept the change minimal to support removal the os.Exit, and that's it.

The other changes, which are good, I feel, are making the current PR uneasy to review.

I would have split this in 2 PRs one that can be easily be merged (the thing about panic), and a refactoring PR that could be reviewed and merged separately.

I'm not suggesting to change anything right now. I report here how I would have done it.
Other reviewers or you could have different opinions.

Also, I can be simply wrong and the changes here are somehow needed to address the os.Exit issue.

Copy link
Author

@deefdragon deefdragon Nov 22, 2025

Choose a reason for hiding this comment

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

I really didn't want to refactor the wait groups & goroutines as I hate the resulting diff due to the tabbing. Unfortunately I honestly felt that it was necessary. 1: It was what allowed the panic calls to propagate up a reasonable stack (being recoverable by the calling user), and 2: it leaves the code in a much more simplified, and thus more readable/maintainable state.

As was with the wait groups, it was literally just synchronous code made excessively complicated to allow being lazy with the watcher.Close(), using the defer instead of just closing where it should be closed on error.

Copy link
Contributor

@ccoVeille ccoVeille Nov 22, 2025

Choose a reason for hiding this comment

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

Thanks for confirming what I thought about the fact the code had no need for such asynchronous thing.

You are right about the stack in the panic, it makes sense.

So here, I would like to suggest you to split the first commit in two.

  • the first one to remove the complexity and keeping the os.Exit with a clear commit message about it's a refactoring made for simplifying over - complicated code
  • then a commit about the change about os.Exit.

(They could be inverted of course)

This way the PR diff will stay unchanged, but at least the history would be way clearer


}

// SetConfigFile explicitly defines the path, name and extension of the config file.
Expand Down