Skip to content

Commit dbb618a

Browse files
cesarfdacesarfda
authored
Adding Gowrapper around coroutines (#1988)
1 parent 338c676 commit dbb618a

File tree

14 files changed

+59
-28
lines changed

14 files changed

+59
-28
lines changed

‎.golangci.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ linters-settings:
3939
msg: do not use logutil.Fatal so that launcher can shut down gracefully
4040
- p: ^panic.*$
4141
msg: do not use panic so that launcher can shut down gracefully
42+
- p: ^go func.*$
43+
msg: use gowrapper.Go() instead of raw goroutines for proper panic handling
4244
sloglint:
4345
kv-only: true
4446
context: "all"

‎cmd/launcher/desktop.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/kolide/launcher/ee/desktop/user/notify"
2020
userserver "github.com/kolide/launcher/ee/desktop/user/server"
2121
"github.com/kolide/launcher/ee/desktop/user/universallink"
22+
"github.com/kolide/launcher/ee/gowrapper"
2223
"github.com/kolide/launcher/pkg/authedclient"
2324
"github.com/kolide/launcher/pkg/log/multislogger"
2425
"github.com/kolide/launcher/pkg/rungroup"
@@ -185,15 +186,20 @@ func runDesktop(_ *multislogger.MultiSlogger, args []string) error {
185186
}, func(err error) {})
186187

187188
// run run group
188-
go func() {
189+
gowrapper.Go(context.TODO(), slogger, func() {
189190
// have to run this in a goroutine because menu needs the main thread
190191
if err := runGroup.Run(); err != nil {
191192
slogger.Log(context.TODO(), slog.LevelError,
192193
"running run group",
193194
"err", err,
194195
)
195196
}
196-
}()
197+
}, func(r any) {
198+
slogger.Log(context.TODO(), slog.LevelError,
199+
"exiting after runGroup panic",
200+
"err", r,
201+
)
202+
})
197203

198204
// if desktop is not enabled at start up, wait for send on show desktop channel
199205
if !*flDesktopEnabled {

‎ee/control/consumers/remoterestartconsumer/remoterestartconsumer.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"time"
1111

1212
"github.com/kolide/launcher/ee/agent/types"
13+
"github.com/kolide/launcher/ee/gowrapper"
1314
)
1415

1516
const (
@@ -79,7 +80,7 @@ func (r *RemoteRestartConsumer) Do(data io.Reader) error {
7980

8081
// Perform the restart by signaling actor shutdown, but delay slightly to give
8182
// the actionqueue a chance to process all actions and store their statuses.
82-
go func() {
83+
gowrapper.Go(context.TODO(), r.slogger, func() {
8384
r.slogger.Log(context.TODO(), slog.LevelInfo,
8485
"received remote restart action for current launcher run ID -- signaling for restart shortly",
8586
"action_run_id", restartAction.RunID,
@@ -100,7 +101,7 @@ func (r *RemoteRestartConsumer) Do(data io.Reader) error {
100101
)
101102
return
102103
}
103-
}()
104+
}, func(err any) {})
104105

105106
return nil
106107
}

‎ee/debug/shipper/shipper.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/kolide/launcher/ee/agent/types"
2424
"github.com/kolide/launcher/ee/consoleuser"
2525
"github.com/kolide/launcher/ee/control"
26+
"github.com/kolide/launcher/ee/gowrapper"
2627
"github.com/kolide/launcher/pkg/launcher"
2728
)
2829

@@ -107,11 +108,11 @@ func (s *shipper) Write(p []byte) (n int, err error) {
107108
// OTOH, if we started request in New() we would know sooner if we had a bad upload url ... :shrug:
108109
s.uploadRequestStarted = true
109110
s.uploadRequestWg.Add(1)
110-
go func() {
111+
gowrapper.Go(context.TODO(), s.knapsack.Slogger(), func() {
111112
defer s.uploadRequestWg.Done()
112113
// will close the body in the close function
113114
s.uploadResponse, s.uploadRequestErr = http.DefaultClient.Do(s.uploadRequest) //nolint:bodyclose
114-
}()
115+
}, func(r any) {})
115116

116117
return s.writer.Write(p)
117118
}

‎ee/debug/shipper/shipper_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ func TestShip(t *testing.T) { //nolint:paralleltest
3333
k := typesMocks.NewKnapsack(t)
3434
k.On("EnrollSecret").Return("")
3535
k.On("EnrollSecretPath").Return("")
36+
k.On("Slogger").Return(multislogger.NewNopLogger())
3637
return k
3738
},
3839
assertion: assert.NoError,
@@ -47,6 +48,7 @@ func TestShip(t *testing.T) { //nolint:paralleltest
4748
k := typesMocks.NewKnapsack(t)
4849
k.On("EnrollSecret").Return("")
4950
k.On("EnrollSecretPath").Return("")
51+
k.On("Slogger").Return(multislogger.NewNopLogger())
5052
return k
5153
},
5254
assertion: assert.NoError,
@@ -63,6 +65,7 @@ func TestShip(t *testing.T) { //nolint:paralleltest
6365

6466
k := typesMocks.NewKnapsack(t)
6567
k.On("EnrollSecret").Return("enroll_secret_value")
68+
k.On("Slogger").Return(multislogger.NewNopLogger())
6669
return k
6770
},
6871
expectSignatureHeaders: true,

‎ee/desktop/runner/runner.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"github.com/kolide/launcher/ee/desktop/user/client"
3030
"github.com/kolide/launcher/ee/desktop/user/menu"
3131
"github.com/kolide/launcher/ee/desktop/user/notify"
32+
"github.com/kolide/launcher/ee/gowrapper"
3233
"github.com/kolide/launcher/ee/presencedetection"
3334
"github.com/kolide/launcher/ee/ui/assets"
3435
"github.com/kolide/launcher/pkg/backoff"
@@ -184,14 +185,14 @@ func New(k types.Knapsack, messenger runnerserver.Messenger, opts ...desktopUser
184185
}
185186

186187
runner.runnerServer = rs
187-
go func() {
188+
gowrapper.Go(context.TODO(), runner.slogger, func() {
188189
if err := runner.runnerServer.Serve(); err != nil && !errors.Is(err, http.ErrServerClosed) {
189190
runner.slogger.Log(context.TODO(), slog.LevelError,
190191
"running monitor server",
191192
"err", err,
192193
)
193194
}
194-
}()
195+
}, func(err any) {})
195196

196197
if runtime.GOOS == "darwin" {
197198
runner.osVersion, err = osversion()
@@ -309,10 +310,10 @@ func (r *DesktopUsersProcessesRunner) DetectPresence(reason string, interval tim
309310
// killDesktopProcesses kills any existing desktop processes
310311
func (r *DesktopUsersProcessesRunner) killDesktopProcesses(ctx context.Context) {
311312
wgDone := make(chan struct{})
312-
go func() {
313+
gowrapper.Go(context.TODO(), r.slogger, func() {
313314
defer close(wgDone)
314315
r.procsWg.Wait()
315-
}()
316+
}, func(err any) {})
316317

317318
shutdownRequestCount := 0
318319
for uid, proc := range r.uidProcs {
@@ -794,7 +795,7 @@ func (r *DesktopUsersProcessesRunner) addProcessTrackingRecordForUser(uid string
794795
// The wait group is needed to prevent races.
795796
func (r *DesktopUsersProcessesRunner) waitOnProcessAsync(uid string, proc *os.Process) {
796797
r.procsWg.Add(1)
797-
go func(username string, proc *os.Process) {
798+
gowrapper.Go(context.TODO(), r.slogger.With("uid", uid, "pid", proc.Pid), func() {
798799
defer r.procsWg.Done()
799800
// waiting here gives the parent a chance to clean up
800801
state, err := proc.Wait()
@@ -807,7 +808,7 @@ func (r *DesktopUsersProcessesRunner) waitOnProcessAsync(uid string, proc *os.Pr
807808
"state", state,
808809
)
809810
}
810-
}(uid, proc)
811+
}, func(err any) {})
811812
}
812813

813814
// determineExecutablePath returns DesktopUsersProcessesRunner.executablePath if it is set,

‎ee/desktop/user/menu/menu_systray.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
package menu
22

33
import (
4+
"context"
45
"sync"
56

7+
"github.com/kolide/launcher/ee/gowrapper"
68
"github.com/kolide/systray"
79
)
810

@@ -120,7 +122,7 @@ func (m *menu) makeActionHandler(item *systray.MenuItem, ap ActionPerformer) {
120122
done := make(chan struct{})
121123
doneChans = append(doneChans, done)
122124

123-
go func() {
125+
gowrapper.Go(context.TODO(), m.slogger, func() {
124126
for {
125127
select {
126128
case <-item.ClickedCh:
@@ -131,5 +133,5 @@ func (m *menu) makeActionHandler(item *systray.MenuItem, ap ActionPerformer) {
131133
return
132134
}
133135
}
134-
}()
136+
}, func(r any) {})
135137
}

‎ee/localserver/server.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/kolide/krypto/pkg/echelper"
2222
"github.com/kolide/launcher/ee/agent"
2323
"github.com/kolide/launcher/ee/agent/types"
24+
"github.com/kolide/launcher/ee/gowrapper"
2425
"github.com/kolide/launcher/ee/presencedetection"
2526
"github.com/kolide/launcher/pkg/osquery"
2627
"github.com/kolide/launcher/pkg/traces"
@@ -253,7 +254,7 @@ func (ls *localServer) Start() error {
253254
var ctx context.Context
254255
ctx, ls.cancel = context.WithCancel(context.Background())
255256

256-
go func() {
257+
gowrapper.Go(ctx, ls.slogger, func() {
257258
var lastRun time.Time
258259

259260
ticker := time.NewTicker(pollInterval)
@@ -278,14 +279,14 @@ func (ls *localServer) Start() error {
278279
}
279280
}
280281
}
281-
}()
282+
}, func(r any) {})
282283

283284
l, err := ls.startListener()
284285
if err != nil {
285286
return fmt.Errorf("starting listener: %w", err)
286287
}
287288

288-
if ls.tlsCerts != nil && len(ls.tlsCerts) > 0 {
289+
if len(ls.tlsCerts) > 0 {
289290
ls.slogger.Log(ctx, slog.LevelDebug,
290291
"using TLS",
291292
)

‎pkg/debug/debug.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"strings"
1818

1919
"github.com/google/uuid"
20+
"github.com/kolide/launcher/ee/gowrapper"
2021
)
2122

2223
const debugPrefix = "/debug/"
@@ -42,13 +43,14 @@ func startDebugServer(addrPath string, slogger *slog.Logger) (*http.Server, erro
4243
return nil, fmt.Errorf("opening socket: %w", err)
4344
}
4445

45-
go func() {
46+
gowrapper.Go(context.TODO(), slogger, func() {
4647
if err := serv.Serve(listener); err != nil && err != http.ErrServerClosed {
4748
slogger.Log(context.TODO(), slog.LevelInfo,
48-
"debug server failed", "err", err,
49+
"debug server failed",
50+
"err", err,
4951
)
5052
}
51-
}()
53+
}, func(r any) {})
5254

5355
url := url.URL{
5456
Scheme: "http",

‎pkg/debug/signal_debug.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"os"
1010
"os/signal"
1111
"syscall"
12+
13+
"github.com/kolide/launcher/ee/gowrapper"
1214
)
1315

1416
const debugSignal = syscall.SIGUSR1
@@ -18,7 +20,7 @@ const debugSignal = syscall.SIGUSR1
1820
func AttachDebugHandler(addrPath string, slogger *slog.Logger) {
1921
sig := make(chan os.Signal, 1)
2022
signal.Notify(sig, debugSignal)
21-
go func() {
23+
gowrapper.Go(context.TODO(), slogger, func() {
2224
for {
2325
// Start server on first signal
2426
<-sig
@@ -45,5 +47,6 @@ func AttachDebugHandler(addrPath string, slogger *slog.Logger) {
4547
"shutdown debug server",
4648
)
4749
}
48-
}()
50+
}, func(r any) {})
51+
4952
}

‎pkg/log/logshipper/logshipper.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/kolide/launcher/ee/agent/flags/keys"
2020
"github.com/kolide/launcher/ee/agent/storage"
2121
"github.com/kolide/launcher/ee/agent/types"
22+
"github.com/kolide/launcher/ee/gowrapper"
2223
"github.com/kolide/launcher/pkg/sendbuffer"
2324
slogmulti "github.com/samber/slog-multi"
2425
)
@@ -114,9 +115,10 @@ func (ls *LogShipper) Ping() {
114115
}
115116

116117
ls.isShippingStarted = true
117-
go func() {
118+
gowrapper.Go(context.TODO(), ls.knapsack.Slogger(), func() {
118119
ls.startShippingChan <- struct{}{}
119-
}()
120+
}, func(r any) {})
121+
120122
}
121123

122124
func (ls *LogShipper) Run() error {

‎pkg/log/logshipper/logshipper_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ func TestLogShipper(t *testing.T) {
4040
knapsack.On("RegisterChangeObserver", mock.Anything, keys.LogShippingLevel, keys.LogIngestServerURL)
4141
knapsack.On("LogShippingLevel").Return("info").Times(5)
4242
knapsack.On("CurrentRunningOsqueryVersion").Return("5.12.3")
43+
knapsack.On("Slogger").Return(multislogger.NewNopLogger())
4344

4445
tokenStore := testKVStore(t, storage.TokenStore.String())
4546
knapsack.On("TokenStore").Return(tokenStore)

‎pkg/rungroup/rungroup.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ func (g *Group) Run() error {
113113
interruptWait := semaphore.NewWeighted(numActors)
114114
for _, a := range g.actors {
115115
interruptWait.Acquire(context.Background(), 1)
116-
go func(a rungroupActor) {
116+
gowrapper.Go(context.TODO(), g.slogger, func() {
117117
defer interruptWait.Release(1)
118118
g.slogger.Log(context.TODO(), slog.LevelDebug,
119119
"interrupting actor",
@@ -124,7 +124,7 @@ func (g *Group) Run() error {
124124
"interrupt complete",
125125
"actor", a.name,
126126
)
127-
}(a)
127+
}, func(r any) {})
128128
}
129129

130130
interruptCtx, interruptCancel := context.WithTimeout(context.Background(), InterruptTimeout)

‎pkg/threadas/threadas.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,14 @@
2121
package threadas
2222

2323
import (
24+
"context"
2425
"fmt"
26+
"log/slog"
2527
"runtime"
2628
"syscall"
2729
"time"
30+
31+
"github.com/kolide/launcher/ee/gowrapper"
2832
)
2933

3034
const (
@@ -75,7 +79,7 @@ func ThreadAs(fn func() error, timeout time.Duration, uid uint32, gid uint32) er
7579
// sequence starting the child and our listener.
7680
errChan := make(chan error, 1)
7781

78-
go func() {
82+
gowrapper.Go(context.TODO(), slog.Default(), func() {
7983
// Calling LockOSThread, without a subsequent Unlock,
8084
// will cause the thread to terminate when the
8185
// goroutine does. This seems simpler than resetting
@@ -95,7 +99,9 @@ func ThreadAs(fn func() error, timeout time.Duration, uid uint32, gid uint32) er
9599
}
96100

97101
errChan <- fn()
98-
}()
102+
}, func(r any) {
103+
errChan <- fmt.Errorf("thread permissions handler panic: %v", r)
104+
})
99105

100106
select {
101107
case err := <-errChan:

0 commit comments

Comments
 (0)