-
Notifications
You must be signed in to change notification settings - Fork 50
Power Event Callback Utils #1350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
VishalDalwadi
wants to merge
3
commits into
develop
Choose a base branch
from
fix/netdesk-conn
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package ncutils | ||
|
|
||
| // RegisterNetworkChangeHandler is a no-op on Darwin. It exists so callers can | ||
| // register a network-change handler without platform-specific build tags; | ||
| // only Windows currently delivers these events. | ||
| func RegisterNetworkChangeHandler(onChange func()) error { | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterNetworkChangeHandler is a no-op on Darwin. | ||
| func UnregisterNetworkChangeHandler() {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package ncutils | ||
|
|
||
| // RegisterNetworkChangeHandler is a no-op on Linux. It exists so callers can | ||
| // register a network-change handler without platform-specific build tags; | ||
| // only Windows currently delivers these events. | ||
| func RegisterNetworkChangeHandler(onChange func()) error { | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterNetworkChangeHandler is a no-op on Linux. | ||
| func UnregisterNetworkChangeHandler() {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package ncutils | ||
|
|
||
| import ( | ||
| "syscall" | ||
| "unsafe" | ||
|
|
||
| "github.com/gravitl/netmaker/logger" | ||
| ) | ||
|
|
||
| const afUnspec = 0 | ||
|
|
||
| var ( | ||
| modIphlpapi = syscall.NewLazyDLL("iphlpapi.dll") | ||
| procNotifyIpInterfaceChange = modIphlpapi.NewProc("NotifyIpInterfaceChange") | ||
| procCancelMibChangeNotify2 = modIphlpapi.NewProc("CancelMibChangeNotify2") | ||
|
|
||
| networkChangeNotifyHandle uintptr | ||
| networkChangeCallbackPtr uintptr | ||
|
|
||
| onNetworkChange func() | ||
| ) | ||
|
|
||
| // RegisterNetworkChangeHandler registers a function to run whenever an IP | ||
| // interface changes state (added, removed, or its parameters change - e.g. | ||
| // coming up or going down, gaining/losing an address). This fires | ||
| // independently of suspend/resume notifications: it catches network changes | ||
| // that happen without any sleep/wake cycle at all (Wi-Fi toggled, cable | ||
| // unplugged, hotspot bounced), and also covers cases like Modern Standby | ||
| // where the network can stay associated across a real sleep and | ||
| // suspend/resume notifications alone wouldn't be enough signal. | ||
| func RegisterNetworkChangeHandler(onChange func()) error { | ||
| UnregisterNetworkChangeHandler() | ||
|
|
||
| onNetworkChange = onChange | ||
|
|
||
| if networkChangeCallbackPtr == 0 { | ||
| networkChangeCallbackPtr = syscall.NewCallback(networkChangeCallback) | ||
| } | ||
|
|
||
| var handle uintptr | ||
| ret, _, _ := procNotifyIpInterfaceChange.Call( | ||
| uintptr(afUnspec), | ||
| networkChangeCallbackPtr, | ||
| 0, | ||
| 0, // InitialNotification = FALSE, don't fire once immediately for existing interfaces. | ||
| uintptr(unsafe.Pointer(&handle)), | ||
| ) | ||
| if ret != 0 { | ||
| return syscall.Errno(ret) | ||
| } | ||
| networkChangeNotifyHandle = handle | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterNetworkChangeHandler unregisters any previously registered | ||
| // network change handler. | ||
| func UnregisterNetworkChangeHandler() { | ||
| if networkChangeNotifyHandle != 0 { | ||
| procCancelMibChangeNotify2.Call(networkChangeNotifyHandle) | ||
| networkChangeNotifyHandle = 0 | ||
| } | ||
| onNetworkChange = nil | ||
| } | ||
|
|
||
| // networkChangeCallback is invoked by Windows on a system thread whenever an | ||
| // IP interface's state changes. It must match | ||
| // PIPINTERFACE_CHANGE_CALLBACK's signature. | ||
| func networkChangeCallback(callerContext, row, notificationType uintptr) uintptr { | ||
| logger.Log(2, "windows network interface change notification received") | ||
| if onNetworkChange != nil { | ||
| onNetworkChange() | ||
| } | ||
| return 0 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package ncutils | ||
|
|
||
| // RegisterPowerEventHandlers is a no-op on Darwin. It exists so callers can | ||
| // register suspend/resume handlers without platform-specific build tags; | ||
| // only Windows currently delivers these events. | ||
| func RegisterPowerEventHandlers(suspend, resumeAutomatic, resumeSuspend func()) error { | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterPowerEventHandlers is a no-op on Darwin. | ||
| func UnregisterPowerEventHandlers() {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package ncutils | ||
|
|
||
| // RegisterPowerEventHandlers is a no-op on Linux. It exists so callers can | ||
| // register suspend/resume handlers without platform-specific build tags; | ||
| // only Windows currently delivers these events. | ||
| func RegisterPowerEventHandlers(suspend, resumeAutomatic, resumeSuspend func()) error { | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterPowerEventHandlers is a no-op on Linux. | ||
| func UnregisterPowerEventHandlers() {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| package ncutils | ||
|
|
||
| import ( | ||
| "syscall" | ||
| "unsafe" | ||
|
|
||
| "github.com/gravitl/netmaker/logger" | ||
| ) | ||
|
|
||
| // Power event types delivered to the suspend/resume callback. | ||
| // See: https://learn.microsoft.com/en-us/windows/win32/power/system-power-status | ||
| const ( | ||
| // PBT_APMSUSPEND - the system is about to suspend. Handlers registered for | ||
| // this event must return quickly (Windows gives very little time, on the | ||
| // order of a couple seconds, before suspending regardless) - do only cheap, | ||
| // local work here, not network I/O. | ||
| PBT_APMSUSPEND = 0x4 | ||
| // PBT_APMRESUMESUSPEND - the user resumed interaction with the system after the system | ||
| // entered a low-power/suspended state due to user activity (e.g. closing a laptop lid | ||
| // and reopening it). | ||
| PBT_APMRESUMESUSPEND = 0x7 | ||
| // PBT_APMRESUMEAUTOMATIC - the system has resumed operation, signaling that the | ||
| // system may have resumed automatically (e.g. from hibernate) without user interaction. | ||
| PBT_APMRESUMEAUTOMATIC = 0x12 | ||
|
|
||
| deviceNotifyCallback = 2 | ||
| ) | ||
|
|
||
| // deviceNotifySubscribeParameters mirrors DEVICE_NOTIFY_SUBSCRIBE_PARAMETERS. | ||
| type deviceNotifySubscribeParameters struct { | ||
| callback uintptr | ||
| context uintptr | ||
| } | ||
|
|
||
| var ( | ||
| modUser32 = syscall.NewLazyDLL("user32.dll") | ||
| procRegisterSuspendResumeNotification = modUser32.NewProc("RegisterSuspendResumeNotification") | ||
| procUnregisterSuspendResumeNotification = modUser32.NewProc("UnregisterSuspendResumeNotification") | ||
|
|
||
| suspendResumeNotifyHandle uintptr | ||
| powerEventCallbackPtr uintptr | ||
|
|
||
| onSuspend func() | ||
| onResumeAutomatic func() | ||
| onResumeSuspend func() | ||
| ) | ||
|
|
||
| // RegisterPowerEventHandlers registers the given functions to run when Windows | ||
| // broadcasts PBT_APMSUSPEND (system about to suspend), PBT_APMRESUMEAUTOMATIC | ||
| // (system resumed, possibly without user interaction), and PBT_APMRESUMESUSPEND | ||
| // (user resumed interaction after suspend). Any handler may be nil to ignore | ||
| // that event. Calling this again replaces any previously registered handlers | ||
| // and re-registers the notification. | ||
| // | ||
| // Note: suspend must return quickly - Windows only allows a couple seconds | ||
| // before it suspends regardless, so the suspend handler should do cheap, | ||
| // local work only, not network I/O. | ||
| func RegisterPowerEventHandlers(suspend, resumeAutomatic, resumeSuspend func()) error { | ||
| UnregisterPowerEventHandlers() | ||
|
|
||
| onSuspend = suspend | ||
| onResumeAutomatic = resumeAutomatic | ||
| onResumeSuspend = resumeSuspend | ||
|
|
||
| if powerEventCallbackPtr == 0 { | ||
| powerEventCallbackPtr = syscall.NewCallback(powerEventCallback) | ||
| } | ||
|
|
||
| params := deviceNotifySubscribeParameters{ | ||
| callback: powerEventCallbackPtr, | ||
| } | ||
|
|
||
| handle, _, err := procRegisterSuspendResumeNotification.Call( | ||
| uintptr(unsafe.Pointer(¶ms)), | ||
| uintptr(deviceNotifyCallback), | ||
| ) | ||
| if handle == 0 { | ||
| return err | ||
| } | ||
| suspendResumeNotifyHandle = handle | ||
| return nil | ||
| } | ||
|
|
||
| // UnregisterPowerEventHandlers unregisters any previously registered power | ||
| // event handlers and stops delivery of suspend/resume notifications. | ||
| func UnregisterPowerEventHandlers() { | ||
| if suspendResumeNotifyHandle != 0 { | ||
| procUnregisterSuspendResumeNotification.Call(suspendResumeNotifyHandle) | ||
| suspendResumeNotifyHandle = 0 | ||
| } | ||
| onSuspend = nil | ||
| onResumeAutomatic = nil | ||
| onResumeSuspend = nil | ||
| } | ||
|
|
||
| // powerEventCallback is invoked by Windows on a system thread when a power | ||
| // event occurs. It must match PDEVICE_NOTIFY_CALLBACK_ROUTINE's signature. | ||
| func powerEventCallback(context, eventType, setting uintptr) uintptr { | ||
| switch eventType { | ||
| case PBT_APMSUSPEND: | ||
| logger.Log(0, "windows power event: PBT_APMSUSPEND (system suspending)") | ||
| if onSuspend != nil { | ||
| onSuspend() | ||
| } | ||
| case PBT_APMRESUMEAUTOMATIC: | ||
| logger.Log(0, "windows power event: PBT_APMRESUMEAUTOMATIC (system resumed)") | ||
| if onResumeAutomatic != nil { | ||
| onResumeAutomatic() | ||
| } | ||
| case PBT_APMRESUMESUSPEND: | ||
| logger.Log(0, "windows power event: PBT_APMRESUMESUSPEND (user resumed interaction after suspend)") | ||
| if onResumeSuspend != nil { | ||
| onResumeSuspend() | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Data race on unsynchronized callback globals (onResumeAutomatic, onResumeSuspend, suspendResumeNotifyHandle) (bug)
The package-level variables
onResumeAutomatic,onResumeSuspend(function pointers declared at lines 38-39), andsuspendResumeNotifyHandle(uintptr at line 35) are accessed concurrently with no synchronization.Writers (Go goroutines):
RegisterPowerEventHandlerswritesonResumeAutomaticandonResumeSuspendat lines 50-51, andsuspendResumeNotifyHandleat line 68.UnregisterPowerEventHandlerswritessuspendResumeNotifyHandleat line 76-77, andonResumeAutomatic/onResumeSuspend(setting them to nil) at lines 79-80.Reader (Windows system thread):
powerEventCallback(line 85) is invoked by Windows on a system thread viasyscall.NewCallback(line 54). It readsonResumeAutomaticat line 89 and calls it at line 90; readsonResumeSuspendat line 94 and calls it at line 95.This is a data race under the Go memory model (Go 1.25 race detector would flag it). The critical danger is the TOCTOU (time-of-check-time-of-use) pattern:
if onResumeAutomatic != nil { onResumeAutomatic() }— ifUnregisterPowerEventHandlersnulls the function pointer between the nil check and the call, a nil function invocation (panic: nil pointer dereference) occurs. Conversely, a freshly registered handler may not be visible to an already-dispatched callback, causing events to be silently dropped.💡 Suggestion: Protect all accesses to
onResumeAutomatic,onResumeSuspend, andsuspendResumeNotifyHandlewith async.RWMutex. The callback (reader) acquires a read lock, snapshots the two function pointers into locals, releases the lock, then calls the non-nil snapshots. Register/Unregister (writers) acquire the write lock.📋 Prompt for AI Agents
In
ncutils/power_windows.go: (1) addvar cbMu sync.RWMutexto the var block on line 30. (2) InRegisterPowerEventHandlers(line 47), holdcbMu.Lock()before theUnregisterPowerEventHandlers()call at line 48, and release it aftersuspendResumeNotifyHandle = handleat line 68. Also reorder: if the syscall fails at line 65-66, restore the handlers to nil before returning so the registration is atomic. (3) InUnregisterPowerEventHandlers(line 74), holdcbMu.Lock()around lines 75-80. (4) InpowerEventCallback(line 85), acquirecbMu.RLock(), snapshotonResumeAutomaticandonResumeSuspendinto local variables, releasecbMu.RUnlock(), then check and call the local snapshots — this avoids holding the lock during user code execution on the system thread.