diff --git a/NETWORK_DELAY_ANALYSIS.md b/NETWORK_DELAY_ANALYSIS.md new file mode 100644 index 00000000..a024a01e --- /dev/null +++ b/NETWORK_DELAY_ANALYSIS.md @@ -0,0 +1,193 @@ +# Network Delay Analysis for NetworkManager v1.13.0 + +## Executive Summary + +This document analyzes the changeset introduced in NetworkManager v1.13.0 (released 2026-01-30) to identify potential network delays. The analysis focuses on three key features: +1. IPv4LL support for Ethernet and WiFi interfaces +2. WiFi enablement upon migration +3. GCancellable implementation to fix async WiFi connection crashes + +## Changeset Overview + +According to the CHANGELOG.md, version 1.13.0 introduced: + +``` +### Added +- Implemented IPv4LL support for both Ethernet and WiFi interfaces +- Implemented logic to enable WiFi upon migration +- Fixed crash of Thunder Plugin upon Async WiFiConnect by using GCancellable +``` + +## Detailed Findings + +### 1. GCancellable Implementation (Crash Fix) + +**Purpose:** Fix crashes during asynchronous WiFi connection operations + +**Location:** `plugin/gnome/NetworkManagerGnomeWIFI.h` and `.cpp` + +**Implementation Details:** +- New member variable: `GCancellable *m_cancellable` (line 105 in header) +- Thread-safe mutex protection: `std::mutex m_cancellableMutex` (line 106 in header) +- Proper lifecycle management: + - Created in `createClientNewConnection()` (NetworkManagerGnomeWIFI.cpp, lines 66-69) + - Cancelled in `deleteClientConnection()` (NetworkManagerGnomeWIFI.cpp, lines 77-84) + +**Network Delay Impact:** +- **✅ MINIMAL IMPACT** - GCancellable is properly implemented with async callbacks +- No blocking operations introduced +- Actually improves reliability by preventing crashes during async operations +- Allows proper cancellation of pending operations without blocking + +### 2. WiFi Migration Logic + +**Purpose:** Enable WiFi automatically during system migration + +**Location:** `plugin/gnome/NetworkManagerGnomeWIFI.cpp` + +**Key Method:** `activateKnownConnection()` (lines ~800-930) + +**Implementation Details:** +- Activates known WiFi connections during migration +- Multiple synchronous wait operations on main loop +- Sets autoconnect to true for saved connections + +**Potential Delay Points:** +```cpp +// Line 830, 898, 918: Multiple blocking wait() calls +wait(m_loop); // Default timeout: 10 seconds (line 63 in header) +``` + +**Network Delay Impact:** +- **⚠️ MODERATE IMPACT** - Multiple blocking waits during migration +- Each `wait()` call has a default 10-second timeout +- Could delay network availability by 10-30 seconds during migration +- Migration process is synchronous, blocking other network operations + +### 3. IPv4LL (Link-Local Addressing) Support + +**Purpose:** Provide automatic IP addressing when DHCP is unavailable + +**Current Status:** +- **❌ NOT FOUND** - No explicit IPv4LL implementation detected in codebase +- Standard DHCP configuration present (lines 606, 614-618 in NetworkManagerGnomeWIFI.cpp) +- IPv4 change callbacks exist but no link-local fallback logic visible +- DHCP timeout set to `INT32_MAX` (infinite) on line 614 + +**Network Delay Impact:** +- **⚠️ POTENTIAL IMPACT** - If IPv4LL was added but not visible in current code +- Link-local addressing typically adds 2-3 seconds for address probing +- Could cause initial connection delays when DHCP is slow/unavailable + +## Critical Delay Issues Discovered + +### Issue #1: WPS Process Synchronous Sleep + +**Location:** `plugin/gnome/NetworkManagerGnomeWIFI.cpp` + +**Problem:** +```cpp +// Line 1683: Blocking 3-second sleep during WPS disconnect +sleep(3); // wait for 3 sec to complete disconnect process + +// Line 1574: Additional sleep in retry loop +sleep(WPS_RETRY_WAIT_IN_MS); // 10 seconds defined in header +``` + +**Impact:** +- **🔴 HIGH IMPACT** - Blocks network thread for 3+ seconds +- Can accumulate to 30+ seconds with retries (WPS_RETRY_COUNT = 10) +- Affects all network operations during WPS connect/disconnect + +### Issue #2: Default Timeout Values + +**Location:** `plugin/gnome/NetworkManagerGnomeWIFI.h` + +**Constants:** +```cpp +#define WPS_RETRY_WAIT_IN_MS 10 // 10 sec +#define WPS_RETRY_COUNT 10 +#define WPS_PROCESS_WAIT_IN_MS 5 + +bool wait(GMainLoop *loop, int timeOutMs = 10000); // 10 second default +``` + +**Impact:** +- **⚠️ MODERATE IMPACT** - 10-second default timeouts +- Can cause perceived delays during normal WiFi operations +- WPS operations can take up to 100 seconds (10 retries × 10 seconds) + +### Issue #3: Multiple Blocking Wait Calls + +**Location:** Throughout `plugin/gnome/NetworkManagerGnomeWIFI.cpp` + +**Occurrences:** +- Line 330: `wait(m_loop);` - WiFi disconnect +- Line 830: `wait(m_loop);` - Known connection activation +- Line 898: `wait(m_loop);` - Connection update +- Line 918: `wait(m_loop);` - Connection activation +- Multiple other instances in connect/disconnect flows + +**Impact:** +- **⚠️ MODERATE IMPACT** - Sequential blocking operations +- Each can take up to 10 seconds +- No parallel operation support during migration + +## Recommendations + +### High Priority + +1. **Replace Synchronous Sleeps:** + - Replace `sleep(3)` with async callback mechanisms + - Use g_timeout_add() for non-blocking delays + - Implement proper state machine for WPS operations + +2. **Reduce Default Timeouts:** + - Lower default timeout from 10 seconds to 5 seconds for interactive operations + - Implement progressive timeout strategies (fast timeout, then retry with longer timeout) + +3. **Implement IPv4LL Properly:** + - If IPv4LL is claimed in v1.13.0, verify implementation exists + - Document IPv4LL configuration and timeout behavior + - Ensure link-local addressing doesn't block DHCP attempts + +### Medium Priority + +1. **Make Migration Async:** + - Convert migration logic to non-blocking async operations + - Implement callback-based activation for known connections + - Allow partial network availability during migration + +2. **Add Timeout Configuration:** + - Make timeout values configurable via configuration file + - Different timeout values for different operation types + - Allow override for low-latency vs. high-reliability scenarios + +### Low Priority + +1. **Optimize WPS Retry Logic:** + - Implement exponential backoff instead of fixed delays + - Add early termination on unrecoverable errors + - Reduce retry count or make it configurable + +## Conclusion + +The v1.13.0 changeset introduces **potential network delays** primarily through: + +1. **WiFi Migration Logic** - Synchronous blocking operations with 10+ second timeouts +2. **WPS Operations** - Multiple blocking sleeps totaling 3-30+ seconds +3. **Missing IPv4LL Implementation** - Claimed feature not visible in code + +The **GCancellable implementation** is well-designed and does NOT introduce delays; it actually improves reliability. + +### Overall Risk Assessment: +- **Migration scenarios:** 10-30 second delays possible ⚠️ +- **WPS operations:** 3-100 second delays possible 🔴 +- **Normal WiFi connect:** 10 second timeout delays possible ⚠️ +- **Crash prevention (GCancellable):** No delay impact ✅ + +### Recommended Actions: +1. Review and optimize WPS sleep() calls immediately +2. Make migration logic asynchronous +3. Verify IPv4LL implementation or update changelog +4. Reduce default timeout values for better user experience diff --git a/SLEEP_3SEC_DELAY.md b/SLEEP_3SEC_DELAY.md new file mode 100644 index 00000000..e56d3575 --- /dev/null +++ b/SLEEP_3SEC_DELAY.md @@ -0,0 +1,105 @@ +# 3-Second Network Delay - Sleep Call Location + +## Summary +A **blocking sleep(3)** call causes a **3-second network delay** during WPS (Wi-Fi Protected Setup) operations. + +## Exact Location + +**File:** `plugin/gnome/NetworkManagerGnomeWIFI.cpp` +**Line:** `1683` +**Function:** `wifiManager::wpsProcess()` + +## Code Context + +```cpp +// Line 1673-1684 +NMDeviceState state = nm_device_get_state(wifidevice); +if(state > NM_DEVICE_STATE_DISCONNECTED) +{ + NMLOG_INFO("stopping the ongoing Wi-Fi connection"); + // some other ssid connected or connecting; wps need a disconnected wifi state + nm_device_disconnect(wifidevice, NULL, &error); + if (error) { + NMLOG_ERROR("disconnect connection failed %s", error->message); + g_error_free(error); + } + sleep(3); // wait for 3 sec to complete disconnect process +} +``` + +## When This Delay Occurs + +This 3-second blocking sleep happens when: +1. **WPS (Wi-Fi Protected Setup)** connection process is running +2. Another WiFi network is already connected or connecting +3. The system needs to disconnect from the current network before WPS can proceed + +## Impact + +### Severity: 🔴 **HIGH** + +- **Duration:** Blocks network thread for exactly **3 seconds** +- **Frequency:** Can occur on **every WPS retry** (up to 10 retries) +- **Cumulative Impact:** Potentially **30 seconds total delay** in worst case +- **Blocking Nature:** This is a **synchronous sleep** - completely blocks the thread +- **User Experience:** WiFi becomes unresponsive during this time + +## Why This Is Problematic + +1. **Synchronous Blocking:** The `sleep(3)` call blocks the entire thread, preventing any network operations +2. **No Async Handling:** Unlike other operations that use callbacks, this is a hard sleep +3. **Fixed Duration:** Always waits 3 seconds regardless of actual disconnect time +4. **Multiple Occurrences:** Can happen multiple times during WPS retry loop + +## Related Code Constants + +**File:** `plugin/gnome/NetworkManagerGnomeWIFI.h` +**Lines:** `35-37` + +```cpp +#define WPS_RETRY_WAIT_IN_MS 10 // 10 sec +#define WPS_RETRY_COUNT 10 +#define WPS_PROCESS_WAIT_IN_MS 5 +``` + +## Full Context - WPS Process Flow + +The `wpsProcess()` function (starting at line 1553) includes: +- A retry loop that runs up to 10 times (WPS_RETRY_COUNT = 10) +- Each retry sleeps for 10 seconds (WPS_RETRY_WAIT_IN_MS) +- If disconnection is needed, adds this additional 3-second sleep + +**Total potential delay in worst case:** +- 10 retries × 10 seconds = 100 seconds (retry delays) +- Plus up to 10 × 3 seconds = 30 seconds (disconnect delays) +- **Maximum: 130 seconds of blocking delays** + +## Recommended Solution + +Replace the synchronous `sleep(3)` with: + +1. **Async callback approach:** Wait for actual disconnect event +2. **Event-driven logic:** Use NetworkManager's state change signals +3. **Timeout with polling:** Use `g_timeout_add()` with shorter intervals +4. **Conditional check:** Verify disconnect completed before proceeding + +Example replacement: +```cpp +// Instead of: sleep(3); +// Use async approach with callback or event monitoring +GSource *source = g_timeout_source_new(100); // Check every 100ms +// Set callback to monitor disconnect state +// Continue when state == NM_DEVICE_STATE_DISCONNECTED or timeout after 3s +``` + +## Additional Sleep Calls in Same File + +For complete picture, other sleep/delay calls: +- **Line 1574:** `sleep(WPS_RETRY_WAIT_IN_MS);` - 10 second sleep per retry +- **Line 1913:** `g_usleep(500 * 1000);` - 500ms microsleep (less critical) + +## References + +- Main analysis document: [NETWORK_DELAY_ANALYSIS.md](NETWORK_DELAY_ANALYSIS.md) +- WPS implementation: `plugin/gnome/NetworkManagerGnomeWIFI.cpp:1553-1800` +- Related issue: WPS disconnect synchronization