From 5d37c126bcb1c7261b2d85a98fe8d08cde0435c6 Mon Sep 17 00:00:00 2001 From: Alexandr Stelnykovych Date: Thu, 26 Jun 2025 17:14:57 +0300 Subject: [PATCH 1/2] Fixed SPN re-initialization https://github.com/safing/portmaster/issues/1872 --- service/mgr/group.go | 10 +++++++++- service/mgr/worker.go | 22 ++++++++++++++++++++++ service/mgr/worker_info.go | 16 ++++++++++++++++ spn/captain/api.go | 2 +- 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/service/mgr/group.go b/service/mgr/group.go index 0019c28c..0a11a021 100644 --- a/service/mgr/group.go +++ b/service/mgr/group.go @@ -206,7 +206,15 @@ func (g *Group) stopFrom(index int) (ok bool) { ok = false } m.mgr.Cancel() - if m.mgr.WaitForWorkers(0) { + + var waitSucceeded bool + if m.mgr.hasStopWorker() { + waitSucceeded = m.mgr.WaitForWorkersFromStop(0) + } else { + waitSucceeded = m.mgr.WaitForWorkers(0) + } + + if waitSucceeded { m.mgr.Info("stopped", "time", time.Since(startTime)) } else { ok = false diff --git a/service/mgr/worker.go b/service/mgr/worker.go index 2151a2c8..55de8e67 100644 --- a/service/mgr/worker.go +++ b/service/mgr/worker.go @@ -8,6 +8,7 @@ import ( "runtime" "runtime/debug" "strings" + "sync/atomic" "time" "github.com/safing/portmaster/base/log" @@ -30,6 +31,12 @@ type WorkerCtx struct { workerMgr *WorkerMgr // TODO: Attach to context instead? logger *slog.Logger + + // isStopWorker indicates whether this worker is responsible for stopping + // the manager. When true, the manager will not wait for this worker to + // finish during stop, preventing deadlocks where the stop worker + // would wait for itself to complete. + isStopWorker atomic.Bool } // AddToCtx adds the WorkerCtx to the given context. @@ -250,6 +257,20 @@ func (m *Manager) manageWorker(name string, fn func(w *WorkerCtx) error) { // - Panic catching. // - Flow control helpers. func (m *Manager) Do(name string, fn func(w *WorkerCtx) error) error { + return m.do(name, false, fn) +} + +// DoAsStopWorker is like Do(...), but marks the worker as a stop worker. +// This means that the manager will not wait for this worker to finish when stopping. +// Only one stop worker can be started at a time. +func (m *Manager) DoAsStopWorker(name string, fn func(w *WorkerCtx) error) error { + if m.hasStopWorker() { + return fmt.Errorf("cannot start stop worker %q: already has a stop worker", name) + } + return m.do(name, true, fn) +} + +func (m *Manager) do(name string, isStopWorker bool, fn func(w *WorkerCtx) error) error { // Create context. w := &WorkerCtx{ name: name, @@ -257,6 +278,7 @@ func (m *Manager) Do(name string, fn func(w *WorkerCtx) error) error { ctx: m.Ctx(), logger: m.logger.With("worker", name), } + w.isStopWorker.Store(isStopWorker) m.workerStart(w) defer m.workerDone(w) diff --git a/service/mgr/worker_info.go b/service/mgr/worker_info.go index 7f044189..877e0ac4 100644 --- a/service/mgr/worker_info.go +++ b/service/mgr/worker_info.go @@ -73,6 +73,22 @@ func (m *Manager) unregisterWorker(w *WorkerCtx) { } } +func (m *Manager) hasStopWorker() bool { + m.workersLock.Lock() + defer m.workersLock.Unlock() + + for _, w := range m.workers { + if w == nil { + continue + } + if w.isStopWorker.Load() { + return true + } + } + + return false +} + // WorkerInfo holds status information about a managers workers. type WorkerInfo struct { Running int diff --git a/spn/captain/api.go b/spn/captain/api.go index cfd38de4..16bf817c 100644 --- a/spn/captain/api.go +++ b/spn/captain/api.go @@ -34,7 +34,7 @@ func handleReInit(ar *api.Request) (msg string, err error) { } // Make sure SPN is stopped and wait for it to complete. - err = module.mgr.Do("stop SPN for re-init", module.instance.SPNGroup().EnsureStoppedWorker) + err = module.mgr.DoAsStopWorker("stop SPN for re-init", module.instance.SPNGroup().EnsureStoppedWorker) if err != nil { return "", fmt.Errorf("failed to stop SPN for re-init: %w", err) } From bcaf0b90f04519c84d2dc0476db15cc91bbdf1da Mon Sep 17 00:00:00 2001 From: Alexandr Stelnykovych Date: Thu, 26 Jun 2025 17:55:10 +0300 Subject: [PATCH 2/2] Refactor WorkerCtx to use a boolean for isStopWorker --- service/mgr/worker.go | 13 ++++++------- service/mgr/worker_info.go | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/service/mgr/worker.go b/service/mgr/worker.go index 55de8e67..8572153e 100644 --- a/service/mgr/worker.go +++ b/service/mgr/worker.go @@ -8,7 +8,6 @@ import ( "runtime" "runtime/debug" "strings" - "sync/atomic" "time" "github.com/safing/portmaster/base/log" @@ -36,7 +35,7 @@ type WorkerCtx struct { // the manager. When true, the manager will not wait for this worker to // finish during stop, preventing deadlocks where the stop worker // would wait for itself to complete. - isStopWorker atomic.Bool + isStopWorker bool } // AddToCtx adds the WorkerCtx to the given context. @@ -273,12 +272,12 @@ func (m *Manager) DoAsStopWorker(name string, fn func(w *WorkerCtx) error) error func (m *Manager) do(name string, isStopWorker bool, fn func(w *WorkerCtx) error) error { // Create context. w := &WorkerCtx{ - name: name, - workFunc: fn, - ctx: m.Ctx(), - logger: m.logger.With("worker", name), + name: name, + workFunc: fn, + ctx: m.Ctx(), + logger: m.logger.With("worker", name), + isStopWorker: isStopWorker, } - w.isStopWorker.Store(isStopWorker) m.workerStart(w) defer m.workerDone(w) diff --git a/service/mgr/worker_info.go b/service/mgr/worker_info.go index 877e0ac4..61c9b86d 100644 --- a/service/mgr/worker_info.go +++ b/service/mgr/worker_info.go @@ -81,7 +81,7 @@ func (m *Manager) hasStopWorker() bool { if w == nil { continue } - if w.isStopWorker.Load() { + if w.isStopWorker { return true } }