Adding Gowrapper around coroutines #1988
Conversation
|
Might be in your TODO already, but can you add it to the linter? |
…error logging for panics
should omit nil check; len() for []crypto/tls.Certificate is defined as zero (S1009)go-staticcheck
… in debug and log shipping
| }, func(r any) { | ||
| slogger.Log(context.TODO(), slog.LevelError, | ||
| "exiting after runGroup panic", | ||
| "err", r, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
I don't think you need to provide a second function to gowrapper.Go just to perform logging -- the gowrapper already handles logging on panic. This onPanic function is only needed if we need to perform an additional action when we notice a panic. (See e.g. https://github.com/kolide/launcher/blob/main/cmd/launcher/svc_windows.go#L205.)
Anywhere in this PR you have an onPanic function that just does logging, I would remove it and replace with func(r any) {}.
There was a problem hiding this comment.
I think just this one got left behind when you were updating 🙂
There was a problem hiding this comment.
My bad, I added that change to the refactor PR
| @@ -79,7 +80,7 @@ func (r *RemoteRestartConsumer) Do(data io.Reader) error { | |||
|
|
|||
| // Perform the restart by signaling actor shutdown, but delay slightly to give | |||
| // the actionqueue a chance to process all actions and store their statuses. | |||
| go func() { | |||
| gowrapper.Go(context.TODO(), r.slogger, func() { | |||
There was a problem hiding this comment.
I love wrapping all our goroutines to help us be aware of panics! I think maybe we could improve the ergonomics of the wrapping so that we don't to pass an empty func if we just want the default onPanic behavior.
Some suggestions in no particular order:
- Having another gowrapper func that doesn't require an OnPanic func.
- Updating gowrapper.Go to do an nil check on the func so we could just pass nil as the 4th param.
- Make the gowrapper into a struct and use an options pattern.
These are just suggestions, I'de also accept the PR as is.
There was a problem hiding this comment.
I like the first option especially, feels really easy to slot in with our existing usage. Maybe Go(ctx context.Context, slogger *slog.Logger, goroutine func()) (for usage that doesn't require an OnPanic func) and GoWithRecoveryAction(ctx context.Context, slogger *slog.Logger, goroutine func(), onPanic func(r any)) (for usage that does require an OnPanic func). I'd be fine with that in a follow-up PR as well.
There was a problem hiding this comment.
Sounds good, I'll merge this one and start with the follow up on a seperate PR
Resolves #1699:
This pull request introduces the use of
gowrapper.Go()for better panic handling across various parts of the codebase. The changes ensure that goroutines are wrapped with proper context and logging for error handling. Below are the most significant changes grouped by theme:Panic Handling with
gowrapper.Go().golangci.yml: Added a new linter rule to usegowrapper.Go()instead of raw goroutines for proper panic handling.cmd/launcher/desktop.go: Replaced raw goroutines withgowrapper.Go()for running the run group and handling errors. [1] [2]ee/control/consumers/remoterestartconsumer/remoterestartconsumer.go: Updated theDomethod to usegowrapper.Go()for executing the restart logic. [1] [2] [3]ee/debug/shipper/shipper.go: Wrapped the upload request goroutine withgowrapper.Go()for better error handling. [1] [2] [3]ee/desktop/runner/runner.go: Usedgowrapper.Go()in multiple places to handle server and process monitoring. [1] [2] [3] [4] [5]Additional Imports for
gowrapperee/desktop/user/menu/menu_systray.go: Addedgowrapperimport and used it for menu action handlers. [1] [2] [3]ee/localserver/server.go: Integratedgowrapper.Go()for starting the local server. [1] [2] [3]pkg/debug/debug.go: Wrapped the debug server goroutine withgowrapper.Go(). [1] [2]pkg/debug/signal_debug.go: Usedgowrapper.Go()for handling debug signals. [1] [2] [3]pkg/log/logshipper/logshipper.go: Updated the log shipper to usegowrapper.Go()for starting the shipping process. [1] [2]Test Updates
ee/debug/shipper/shipper_test.go: Added mock returns forSloggerin tests. [1] [2] [3]pkg/log/logshipper/logshipper_test.go: IncludedSloggermock in the log shipper tests.Miscellaneous
pkg/rungroup/rungroup.go: Replaced raw goroutines withgowrapper.Go()for interrupting actors. [1] [2]pkg/threadas/threadas.go: Usedgowrapper.Go()for threading operations. [1] [2]