canonical / pebble

Pebble is a lightweight Linux service manager with layered configuration and an HTTP API.
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
146 stars 54 forks source link

fix(run): don't overwrite err on reaper.Stop() #390

Closed thp-canonical closed 6 months ago

thp-canonical commented 6 months ago

PR #380 recently changed the way the reaper is stopped.

If runDaemon() returns an error the defer method overwrites the err return value in err = reaper.Stop(). If err was already set to something else, this will in effect overwrite the non-nil err with either nil (if reaper.Stop() returns nil) or a different error (if reaper.Stop() returns a different error).

Currently (without this PR):

With this PR:

Because this defer method is the first one in the function, it will get called last, so it's the one that will have err set on entry if the function returns a non-nil error.

thp-canonical commented 6 months ago

Instead of ignoring the error from reaper.Stop in the last case, how about we just log the reaper.Stop error in all cases? It's during shutdown in any case, so it doesn't actually matter as things will get cleaned up (though currently reaper.Stop never returns an error). Can we do this?

Sounds reasonable, PR updated - thanks! :)

benhoyt commented 6 months ago

@thp-canonical Looks like you just need to replace %w with %v in the logger call to fix the tests.