-
Notifications
You must be signed in to change notification settings - Fork 275
[AI Task] [Tizen.Network.WiFi] Consolidate WiFiManagerImpl async boilerplate into RunOneShotAsync helper#7679
[AI Task] [Tizen.Network.WiFi] Consolidate WiFiManagerImpl async boilerplate into RunOneShotAsync helper #7679JoonghyunCho wants to merge 2 commits into
Conversation
...to RunOneShotAsync helper The 8 one-shot async operations in WiFiManagerImpl (Activate, ActivateWithWiFiPickerTested, Deactivate, Scan, ScanSpecificAP, BssidScan, HiddenAPConnect, StartMultiScan) each repeated the same ~45-line TaskCompletionSource + callback-map registration + locked native call + error-mapping boilerplate, differing only by the native interop call and the exception produced on failure. Extracts a private RunOneShotAsync helper that captures the common structure and takes the operation name, the native call, and an exception factory as parameters. Each method becomes a single expression. Callback registration/removal order, lock boundaries, CheckReturnValue usage, and the exact per-method exception type and message are all preserved, so behavior is unchanged. The WiFiAP async methods named in #7621 are intentionally left for a follow-up: ConnectWpsAsync uses a separate _wpsTaskMap and type dispatch, and ConnectAsync/DisconnectAsync/ForgetAPAsync use divergent disposal checks and inline _apHandle error handling with inconsistent locking, so they do not fit this helper without changing behavior. Fixes #7621
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.
Code Review
This pull request refactors several asynchronous WiFi operations in WiFiManagerImpl.cs (including activation, deactivation, scanning, and connection) by introducing a shared helper method, RunOneShotAsync, to eliminate repetitive boilerplate code. A review comment identifies a potential memory leak in this helper method: if the synchronous native call fails or an exception is thrown before the callback is invoked, the callback is never removed from _callback_map. A fix is suggested to clean up the callback in the catch block.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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.
If nativeCall fails synchronously or CheckReturnValue throws an exception, the registered callback is never removed from _callback_map. Since the native operation failed to initiate, the native callback will never be invoked to clean itself up. This results in a memory leak where the callback delegate (and any captured variables like task) remains in _callback_map indefinitely.
To prevent this leak, we should remove the callback from _callback_map inside the catch block.
catch (Exception e) { Log.Error(Globals.LogTag, $"Exception on {opName}\n{e}"); lock (_callback_map) { _callback_map.Remove(id); } task.SetException(e); }
JoonghyunCho
commented
Jun 7, 2026
🤖 [AI Review]
Reviewed — no findings.
Scope checked:
- All 8 converted async methods preserve their
CheckReturnValueoperation name (Activate,ActivateWithWiFiPickerTested,Deactivate,Scan,ScanSpecificAP,BssidScan,HiddenAPConnect,MultiScan). - The delegate handed to native is the stored
_callback_map[id]reference (not a freshly-allocated lambda), so it stays GC-rooted by the map until the callback removes it — delegate-lifetime contract unchanged. - Exact per-method exception type and message are preserved via
exceptionFactory:WiFiErrorFactory.GetException(...)for Activate/Deactivate,InvalidOperationExceptionwith the original text for the scan/connect family. lock (_callback_map)boundaries around both callback registration and the native call are unchanged;GetSafeHandle()is still invoked inside the lock at call time.- All 8 methods are
internalandRunOneShotAsyncisprivate→ no public API surface, so no XML-doc/since_tizen obligation.
No 🔴 critical issues, no 🟡 suggestions to flag.
Automated review — final merge decision rests with human reviewers.
Remove the registered callback from _callback_map inside the RunOneShotAsync catch block so a synchronous native-call/CheckReturnValue failure no longer leaks the callback delegate (and its captured task). Applied-Human-Comments: 3369215841
JoonghyunCho
commented
Jun 8, 2026
🤖 [AI Review]
Addressed review feedback in commit d0d4f97. Summary: RunOneShotAsync now removes the registered callback from _callback_map inside its catch block, so a synchronous failure of the native call or CheckReturnValue no longer leaks the callback delegate and its captured task.
Summary
Consolidates the repeated one-shot async boilerplate in
WiFiManagerImplinto a single privateRunOneShotAsynchelper. The 8 affected methods each repeated ~45 lines of nearly identicalTaskCompletionSource+ callback-map registration + locked native call + error mapping, differing only by the native interop call and the exception produced on failure.Changes
src/Tizen.Network.WiFi/Tizen.Network.WiFi/WiFiManagerImpl.csRunOneShotAsync(opName, nativeCall, exceptionFactory)helper capturing the common structure.ActivateAsync,ActivateWithWiFiPickerTestedAsync,DeactivateAsync,ScanAsync,ScanSpecificAPAsync,BssidScanAsync,HiddenAPConnectAsync,StartMultiScan.Behavior preservation
lock (_callback_map)boundaries, andCheckReturnValueusage are unchanged.exceptionFactoryargument (WiFiErrorFactory.GetException(...)for Activate/Deactivate;new InvalidOperationException("...")for the scan/connect family).TaskCompletionSource<bool>is left as-is (noRunContinuationsAsynchronously) so task scheduling semantics are unchanged. TheRunContinuationsAsynchronouslydirection is tracked separately in [AI Refactoring] WiFi 비동기 메서드 13곳 TaskCompletionSource 의 RunContinuationsAsynchronously 누락 → 네이티브 콜백 스레드 인라인 컨티뉴에이션 차단 [Scope: src/Tizen.Network.WiFi] (2026年05月10日) #7620 .Scope note
The
WiFiAPasync methods mentioned in the issue are intentionally deferred to a follow-up.ConnectWpsAsyncuses a separate_wpsTaskMapandWpsInfotype dispatch, andConnectAsync/DisconnectAsync/ForgetAPAsyncuse divergent disposal checks and inline_apHandleerror handling with inconsistent locking — they cannot fold into this helper without changing behavior. The issue explicitly sanctions splitting WiFiManagerImpl and WiFiAP into separate PRs.Mode
Refactoring (behavior-preserving)
Verification
dotnet build src/Tizen.Network.WiFi/Tizen.Network.WiFi.csproj -c Release, 0 errors)sdbdevice connected —sdb devicesreports no targets; manual benchmark verification required)Fixes #7621