Refactor sleep detector: bundle runloop state into session struct

client/internal/sleep/detector_darwin.go

@@ -38,6 +38,15 @@ type cfFuncs struct {
 	CFRunLoopRemoveSource func(rl, source, mode uintptr)
 }
 
+// runLoopSession bundles the handles owned by one CFRunLoop lifetime. A nil
+// session means no runloop is active and the next Register must start one.
+type runLoopSession struct {
+	rl       uintptr
+	port     uintptr
+	notifier uintptr
+	rp       uintptr
+}
+
 var (
 	ioKit         iokitFuncs
 	cf            cfFuncs
@@ -51,19 +60,11 @@ var (
 
 	serviceRegistry   = make(map[*Detector]struct{})
 	serviceRegistryMu sync.Mutex
+	session           *runLoopSession
 
-	// lifecycleMu serializes Register and Deregister so concurrent lifecycle
+	// lifecycleMu serializes Register/Deregister so a new registration can't
-	// transitions can't interleave (e.g. a new registration starting a second
+	// start a second runloop while a previous teardown is still pending.
-	// runloop while the previous teardown is still pending).
 	lifecycleMu sync.Mutex
-
-	// runtime state, protected by serviceRegistryMu
-	runLoopRef   uintptr
-	notifyPort   uintptr
-	notifierObj  uintptr
-	rootPort     uintptr
-	runLoopReady chan struct{}
-	runLoopErr   error
 )
 
 func initLibs() error {
@@ -111,10 +112,9 @@ func initLibs() error {
 	return libInitErr
 }
 
-// powerCallback is the IOServiceInterestCallback trampoline. It runs on the
+// powerCallback is the IOServiceInterestCallback trampoline, invoked on the
-// runloop thread (the OS-locked goroutine in runRunLoop). All args are
+// runloop thread. A Go panic crossing the purego boundary has undefined
-// word-sized so purego can forward them without conversion. A Go panic
+// behavior, so contain it here.
-// crossing the purego boundary has undefined behavior, so contain it here.
 func powerCallback(refcon, service, messageType, messageArgument uintptr) uintptr {
 	defer func() {
 		if r := recover(); r != nil {
@@ -123,12 +123,10 @@ func powerCallback(refcon, service, messageType, messageArgument uintptr) uintpt
 	}()
 	switch messageType {
 	case kIOMessageCanSystemSleep:
-		// Consent query that precedes idle sleep; not acknowledging
+		// Not acknowledging forces a 30s IOKit timeout before idle sleep.
-		// forces a 30s IOKit timeout before sleep proceeds.
 		allowPowerChange(messageArgument)
 	case kIOMessageSystemWillSleep:
 		dispatchEvent(EventTypeSleep)
-		// Must acknowledge so the system proceeds with sleep.
 		allowPowerChange(messageArgument)
 	case kIOMessageSystemHasPoweredOn:
 		dispatchEvent(EventTypeWakeUp)
@@ -138,7 +136,10 @@ func powerCallback(refcon, service, messageType, messageArgument uintptr) uintpt
 
 func allowPowerChange(messageArgument uintptr) {
 	serviceRegistryMu.Lock()
-	port := rootPort
+	var port uintptr
+	if session != nil {
+		port = session.rp
+	}
 	serviceRegistryMu.Unlock()
 	if port != 0 {
 		ioKit.IOAllowPowerChange(port, messageArgument)
@@ -183,31 +184,23 @@ func (d *Detector) Register(callback func(event EventType)) error {
 		serviceRegistryMu.Unlock()
 		return fmt.Errorf("detector service already registered")
 	}
-
 	d.callback = callback
 	d.done = make(chan struct{})
 	serviceRegistry[d] = struct{}{}
-
+	needSetup := session == nil
-	if len(serviceRegistry) > 1 {
-		ready := runLoopReady
-		serviceRegistryMu.Unlock()
-		if ready != nil {
-			<-ready
-		}
-		return d.rollbackIfRunLoopFailed()
-	}
-
-	runLoopReady = make(chan struct{})
-	runLoopErr = nil
-	ready := runLoopReady
 	serviceRegistryMu.Unlock()
 
-	go runRunLoop()
+	if !needSetup {
-	<-ready
+		return nil
+	}
 
-	if err := d.rollbackIfRunLoopFailed(); err != nil {
+	errCh := make(chan error, 1)
+	go runRunLoop(errCh)
+	if err := <-errCh; err != nil {
 		serviceRegistryMu.Lock()
-		runLoopReady = nil
+		delete(serviceRegistry, d)
+		close(d.done)
+		d.done = nil
 		serviceRegistryMu.Unlock()
 		return err
 	}
@@ -216,21 +209,6 @@ func (d *Detector) Register(callback func(event EventType)) error {
 	return nil
 }
 
-// rollbackIfRunLoopFailed removes the detector from the registry and returns
-// the runloop setup error if one occurred. Must be called after runLoopReady
-// has been closed.
-func (d *Detector) rollbackIfRunLoopFailed() error {
-	serviceRegistryMu.Lock()
-	err := runLoopErr
-	if err != nil {
-		delete(serviceRegistry, d)
-		close(d.done)
-		d.done = nil
-	}
-	serviceRegistryMu.Unlock()
-	return err
-}
-
 // Deregister removes the detector. When the last detector leaves, IOKit
 // notifications are torn down and the runloop is stopped.
 func (d *Detector) Deregister() error {
@@ -242,7 +220,6 @@ func (d *Detector) Deregister() error {
 		serviceRegistryMu.Unlock()
 		return nil
 	}
-
 	close(d.done)
 	delete(serviceRegistry, d)
 
@@ -250,53 +227,35 @@ func (d *Detector) Deregister() error {
 		serviceRegistryMu.Unlock()
 		return nil
 	}
-	ready := runLoopReady
+	sess := session
+	session = nil
 	serviceRegistryMu.Unlock()
 
 	log.Info("sleep detection service stopping (deregister)")
 
-	// Wait for the runloop setup to publish its state before we read it.
+	if sess == nil {
-	// If setup already failed, the fields stayed zero and the checks below
+		return nil
-	// become no-ops.
-	if ready != nil {
-		<-ready
 	}
 
-	serviceRegistryMu.Lock()
+	// CFRunLoop and IOKit teardown calls are safe from a non-runloop thread.
-	rl := runLoopRef
+	if sess.rl != 0 && sess.port != 0 {
-	port := notifyPort
+		source := ioKit.IONotificationPortGetRunLoopSource(sess.port)
-	notifier := notifierObj
+		cf.CFRunLoopRemoveSource(sess.rl, source, cfCommonModes)
-	rp := rootPort
-	serviceRegistryMu.Unlock()
-
-	// CFRunLoopStop and CFRunLoopRemoveSource are thread-safe; deregistering
-	// notifications from another thread is allowed by IOKit.
-	if rl != 0 && port != 0 {
-		source := ioKit.IONotificationPortGetRunLoopSource(port)
-		cf.CFRunLoopRemoveSource(rl, source, cfCommonModes)
 	}
-	if notifier != 0 {
+	if sess.notifier != 0 {
-		n := notifier
+		n := sess.notifier
 		ioKit.IODeregisterForSystemPower(&n)
 	}
-	if rp != 0 {
+	if sess.rp != 0 {
-		ioKit.IOServiceClose(rp)
+		ioKit.IOServiceClose(sess.rp)
 	}
-	if port != 0 {
+	if sess.port != 0 {
-		ioKit.IONotificationPortDestroy(port)
+		ioKit.IONotificationPortDestroy(sess.port)
 	}
-	if rl != 0 {
+	if sess.rl != 0 {
-		cf.CFRunLoopStop(rl)
+		cf.CFRunLoopStop(sess.rl)
 	}
 
-	serviceRegistryMu.Lock()
-	runLoopRef = 0
-	notifyPort = 0
-	notifierObj = 0
-	rootPort = 0
-	runLoopReady = nil
-	serviceRegistryMu.Unlock()
-
 	return nil
 }
 
@@ -329,59 +288,49 @@ func (d *Detector) triggerCallback(event EventType) {
 	}
 }
 
-// runRunLoop registers IOKit notifications and blocks on CFRunLoopRun.
+// runRunLoop owns the OS-locked thread that CFRunLoop is pinned to. Setup
-// Must own a locked OS thread because CFRunLoop is thread-affine. Publishes
+// result is reported on errCh so Register can surface failures synchronously.
-// runloop state to the package globals, then signals runLoopReady. On setup
+func runRunLoop(errCh chan<- error) {
-// failure runLoopErr is set and the goroutine exits without entering the
-// runloop.
-func runRunLoop() {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	// Ensure runLoopReady is closed even on panic so Register/Deregister
+	sess, err := setupSession()
-	// waiters don't hang.
+	if err == nil {
+		serviceRegistryMu.Lock()
+		session = sess
+		serviceRegistryMu.Unlock()
+	}
+	errCh <- err
+	if err != nil {
+		return
+	}
+
 	defer func() {
 		if r := recover(); r != nil {
-			serviceRegistryMu.Lock()
-			runLoopErr = fmt.Errorf("panic during runloop setup: %v", r)
-			ready := runLoopReady
-			serviceRegistryMu.Unlock()
-			if ready != nil {
-				select {
-				case <-ready:
-					// already closed
-				default:
-					close(ready)
-				}
-			}
 			log.Errorf("panic in sleep runloop: %v", r)
 		}
 	}()
+	cf.CFRunLoopRun()
+}
 
-	var portRef uintptr
+// setupSession performs the IOKit registration on the current thread.
-	var notifier uintptr
+// Panics are converted to errors so runRunLoop never leaves errCh unsent.
+func setupSession() (s *runLoopSession, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("panic during runloop setup: %v", r)
+		}
+	}()
+
+	var portRef, notifier uintptr
 	rp := ioKit.IORegisterForSystemPower(0, &portRef, callbackThunk, &notifier)
 	if rp == 0 {
-		serviceRegistryMu.Lock()
+		return nil, fmt.Errorf("IORegisterForSystemPower returned zero")
-		runLoopErr = fmt.Errorf("IORegisterForSystemPower returned zero")
-		ready := runLoopReady
-		serviceRegistryMu.Unlock()
-		close(ready)
-		return
 	}
 
 	rl := cf.CFRunLoopGetCurrent()
 	source := ioKit.IONotificationPortGetRunLoopSource(portRef)
 	cf.CFRunLoopAddSource(rl, source, cfCommonModes)
 
-	serviceRegistryMu.Lock()
+	return &runLoopSession{rl: rl, port: portRef, notifier: notifier, rp: rp}, nil
-	runLoopRef = rl
-	notifyPort = portRef
-	notifierObj = notifier
-	rootPort = rp
-	ready := runLoopReady
-	serviceRegistryMu.Unlock()
-	close(ready)
-
-	cf.CFRunLoopRun()
 }