Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ namespace Microsoft.Maui.Networking
partial class ConnectivityImplementation : IConnectivity
{
#if !(MACCATALYST || MACOS)
// TODO: Use NWPathMonitor on > iOS 12
#pragma warning disable BI1234, CA1416 // Analyzer bug https://github.com/dotnet/roslyn-analyzers/issues/5938
static readonly Lazy<CTCellularData> cellularData = new Lazy<CTCellularData>(() => new CTCellularData());

Expand Down Expand Up @@ -41,7 +40,6 @@ public NetworkAccess NetworkAccess
{
var restricted = false;
#if !(MACCATALYST || MACOS)
// TODO: Use NWPathMonitor on > iOS 12
#pragma warning disable BI1234, CA1416 // Analyzer bug https://github.com/dotnet/roslyn-analyzers/issues/5938
restricted = CellularData.RestrictedState == CTCellularDataRestrictedState.Restricted;
#pragma warning restore BI1234, CA1416
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
using System;
using System.Collections.Generic;
using System.Net;
using System.Threading.Tasks;
#if !(MACCATALYST || MACOS)
using CoreTelephony;
#endif
using CoreFoundation;
using SystemConfiguration;
using Network;

namespace Microsoft.Maui.Networking
{
Expand All @@ -19,144 +18,131 @@ enum NetworkStatus

static class Reachability
{
internal const string HostName = "www.microsoft.com";
static NWPathMonitor sharedMonitor;
static readonly object monitorLock = new object();
static volatile bool pathInitialized = false;

internal static NetworkStatus RemoteHostStatus()
static NWPathMonitor SharedMonitor
{
using (var remoteHostReachability = new NetworkReachability(HostName))
get
{
var reachable = remoteHostReachability.TryGetFlags(out var flags);
lock (monitorLock)
{
if (sharedMonitor == null)
{
sharedMonitor = new NWPathMonitor();

// Set up a handler to track when path becomes available
Action<NWPath> initHandler = null;
initHandler = (path) =>
{
pathInitialized = true;
// Remove the handler after first call to avoid keeping reference
sharedMonitor.SnapshotHandler = null;
};
sharedMonitor.SnapshotHandler = initHandler;

Copilot AI Nov 14, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: The handler nulls itself on line 41, which could happen before the while loop on line 50 checks pathInitialized. If the handler is invoked on the dispatch queue between when pathInitialized is set to true and when the handler is nulled, the logic is correct. However, if there's any delay, the spin-wait loop might still be spinning when the handler is already null, though this is unlikely to cause issues in practice since pathInitialized is set first. A safer approach would be to null the handler outside the lock after the wait completes, or use a ManualResetEventSlim for proper synchronization.

Copilot uses AI. Check for mistakes.

sharedMonitor.SetQueue(DispatchQueue.DefaultGlobalQueue);
sharedMonitor.Start();

// Wait synchronously for the first path update (up to 5 seconds)
var timeout = DateTime.UtcNow.AddSeconds(5);
while (!pathInitialized && DateTime.UtcNow < timeout)
{
System.Threading.Thread.Sleep(10);
}

Copilot AI Nov 14, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spin-wait with Thread.Sleep is an anti-pattern for synchronization. Consider using ManualResetEventSlim or SemaphoreSlim for proper signaling instead of polling. This would be more efficient and eliminate the 10ms polling overhead: var initEvent = new ManualResetEventSlim(false); set in the handler, then initEvent.Wait(TimeSpan.FromSeconds(5)).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spin-wait inside a lock is an anti-pattern. Every thread accessing SharedMonitor blocks for up to 5 seconds during first initialization. Use ManualResetEventSlim instead and move the wait outside the lock:

static readonly ManualResetEventSlim s_initEvent = new(false);

static NWPathMonitor SharedMonitor
{
    get
    {
        bool needsWait = false;
        lock (monitorLock)
        {
            if (sharedMonitor == null)
            {
                sharedMonitor = new NWPathMonitor();
                sharedMonitor.SnapshotHandler = _ => s_initEvent.Set();
                sharedMonitor.SetQueue(DispatchQueue.DefaultGlobalQueue);
                sharedMonitor.Start();
                needsWait = true;
            }
        }
        if (needsWait)
            s_initEvent.Wait(TimeSpan.FromSeconds(5));
        return sharedMonitor;
    }
}

This lets other threads proceed as soon as the monitor is created, and only the first caller waits for initialization.

}
return sharedMonitor;
}
}
}

if (!reachable)
return NetworkStatus.NotReachable;
static NWPath GetCurrentPath()
{
var monitor = SharedMonitor;
return monitor?.CurrentPath;
}

if (!IsReachableWithoutRequiringConnection(flags))
return NetworkStatus.NotReachable;
internal static NetworkStatus RemoteHostStatus()
{
var path = GetCurrentPath();
if (path == null || path.Status != NWPathStatus.Satisfied)
return NetworkStatus.NotReachable;

#if __IOS__
if ((flags & NetworkReachabilityFlags.IsWWAN) != 0)
return NetworkStatus.ReachableViaCarrierDataNetwork;
if (path.UsesInterfaceType(NWInterfaceType.Cellular))
Comment thread
kubaflo marked this conversation as resolved.
return NetworkStatus.ReachableViaCarrierDataNetwork;
#endif

return NetworkStatus.ReachableViaWiFiNetwork;
}
return NetworkStatus.ReachableViaWiFiNetwork;
}

internal static NetworkStatus InternetConnectionStatus()
{
var status = NetworkStatus.NotReachable;

var defaultNetworkAvailable = IsNetworkAvailable(out var flags);
var path = GetCurrentPath();
if (path == null || path.Status != NWPathStatus.Satisfied)
return NetworkStatus.NotReachable;

#if __IOS__
// If it's a WWAN connection..
if ((flags & NetworkReachabilityFlags.IsWWAN) != 0)
status = NetworkStatus.ReachableViaCarrierDataNetwork;
if (path.UsesInterfaceType(NWInterfaceType.Cellular))
return NetworkStatus.ReachableViaCarrierDataNetwork;
#endif

// If the connection is reachable and no connection is required, then assume it's WiFi
if (defaultNetworkAvailable)
{
status = NetworkStatus.ReachableViaWiFiNetwork;
}

// If the connection is on-demand or on-traffic and no user intervention
// is required, then assume WiFi.
if (((flags & NetworkReachabilityFlags.ConnectionOnDemand) != 0 || (flags & NetworkReachabilityFlags.ConnectionOnTraffic) != 0) &&
(flags & NetworkReachabilityFlags.InterventionRequired) == 0)
{
status = NetworkStatus.ReachableViaWiFiNetwork;
}

return status;
return NetworkStatus.ReachableViaWiFiNetwork;
}

internal static IEnumerable<NetworkStatus> GetActiveConnectionType()
{
var status = new List<NetworkStatus>();
var path = GetCurrentPath();

var defaultNetworkAvailable = IsNetworkAvailable(out var flags);
if (path == null || path.Status != NWPathStatus.Satisfied)
return status;

#if __IOS__
// If it's a WWAN connection.
if ((flags & NetworkReachabilityFlags.IsWWAN) != 0)
if (path.UsesInterfaceType(NWInterfaceType.Cellular))
{
status.Add(NetworkStatus.ReachableViaCarrierDataNetwork);
}
else if (defaultNetworkAvailable)
else if (path.UsesInterfaceType(NWInterfaceType.Wifi) || path.UsesInterfaceType(NWInterfaceType.Wired))
#else
// If the connection is reachable and no connection is required, then assume it's WiFi
if (defaultNetworkAvailable)
if (path.UsesInterfaceType(NWInterfaceType.Wifi) || path.UsesInterfaceType(NWInterfaceType.Wired))
#endif
{
status.Add(NetworkStatus.ReachableViaWiFiNetwork);
}
else if (((flags & NetworkReachabilityFlags.ConnectionOnDemand) != 0 || (flags & NetworkReachabilityFlags.ConnectionOnTraffic) != 0) &&
(flags & NetworkReachabilityFlags.InterventionRequired) == 0)
{
// If the connection is on-demand or on-traffic and no user intervention
// is required, then assume WiFi.
status.Add(NetworkStatus.ReachableViaWiFiNetwork);
}

return status;
}

internal static bool IsNetworkAvailable(out NetworkReachabilityFlags flags)
internal static bool IsNetworkAvailable()
{
var ip = new IPAddress(0);
using (var defaultRouteReachability = new NetworkReachability(ip))
{
if (!defaultRouteReachability.TryGetFlags(out flags))
return false;

return IsReachableWithoutRequiringConnection(flags);
}
}

internal static bool IsReachableWithoutRequiringConnection(NetworkReachabilityFlags flags)
{
// Is it reachable with the current network configuration?
var isReachable = (flags & NetworkReachabilityFlags.Reachable) != 0;

// Do we need a connection to reach it?
var noConnectionRequired = (flags & NetworkReachabilityFlags.ConnectionRequired) == 0;

#if __IOS__
// Since the network stack will automatically try to get the WAN up,
// probe that
if ((flags & NetworkReachabilityFlags.IsWWAN) != 0)
noConnectionRequired = true;
#endif

return isReachable && noConnectionRequired;
var path = GetCurrentPath();
return path != null && path.Status == NWPathStatus.Satisfied;
Comment thread
jfversluis marked this conversation as resolved.
}
}

class ReachabilityListener : IDisposable
{
NetworkReachability defaultRouteReachability;
NetworkReachability remoteHostReachability;
// Delay to allow connection status to stabilize before notifying listeners
const int ConnectionStatusChangeDelayMs = 100;

NWPathMonitor pathMonitor;
Action<NWPath> pathUpdateHandler;

internal ReachabilityListener()
{
var ip = new IPAddress(0);
defaultRouteReachability = new NetworkReachability(ip);
#pragma warning disable CA1422 // obsolete in MacCatalyst 15, iOS 13
defaultRouteReachability.SetNotification(OnChange);
defaultRouteReachability.Schedule(CFRunLoop.Main, CFRunLoop.ModeDefault);
#pragma warning restore CA1422

remoteHostReachability = new NetworkReachability(Reachability.HostName);

// Need to probe before we queue, or we wont get any meaningful values
// this only happens when you create NetworkReachability from a hostname
remoteHostReachability.TryGetFlags(out var flags);

#pragma warning disable CA1422 // obsolete in MacCatalyst 15, iOS 13
remoteHostReachability.SetNotification(OnChange);
remoteHostReachability.Schedule(CFRunLoop.Main, CFRunLoop.ModeDefault);
#pragma warning restore CA1422
pathMonitor = new NWPathMonitor();
pathUpdateHandler = async (NWPath path) =>
{
// Add in artificial delay so the connection status has time to change
await Task.Delay(ConnectionStatusChangeDelayMs);
ReachabilityChanged?.Invoke();

Copilot AI Nov 14, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using async void lambda (assigned to Action) creates fire-and-forget behavior where exceptions thrown after the await will be unhandled and could crash the application. Consider wrapping the async work in a non-async lambda that handles the task explicitly, or add try-catch around the entire body to ensure exceptions don't escape.

Suggested change
// Add in artificial delay so the connection status has time to change
await Task.Delay(ConnectionStatusChangeDelayMs);
ReachabilityChanged?.Invoke();
try
{
// Add in artificial delay so the connection status has time to change
await Task.Delay(ConnectionStatusChangeDelayMs);
ReachabilityChanged?.Invoke();
}
catch (Exception ex)
{
// Optionally log the exception, or ignore
System.Diagnostics.Debug.WriteLine($"Exception in ReachabilityListener pathUpdateHandler: {ex}");
}

Copilot uses AI. Check for mistakes.
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a Action it would be ideal to have a method instead. It can be async void if needed here as there is an awaited Task.Delay

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async void lambda — unhandled exceptions will crash the app. Since this is assigned to Action<NWPath>, it's implicitly async void. Per @Cheesebaron's earlier (unresolved) feedback, extract this to a named method and add a try-catch:

async void OnPathUpdate(NWPath path)
{
    try
    {
        await Task.Delay(ConnectionStatusChangeDelayMs);
        ReachabilityChanged?.Invoke();
    }
    catch (Exception ex)
    {
        System.Diagnostics.Debug.WriteLine(
            $"ReachabilityListener handler failed: {ex}");
    }
}

Then: pathMonitor.SnapshotHandler = OnPathUpdate;


pathMonitor.SnapshotHandler = pathUpdateHandler;
pathMonitor.SetQueue(DispatchQueue.DefaultGlobalQueue);
pathMonitor.Start();

#if !(MACCATALYST || MACOS)
#pragma warning disable BI1234, CA1416 // Analyzer bug https://github.com/dotnet/roslyn-analyzers/issues/5938
Expand All @@ -171,10 +157,14 @@ internal ReachabilityListener()

internal void Dispose()
{
defaultRouteReachability?.Dispose();
defaultRouteReachability = null;
remoteHostReachability?.Dispose();
remoteHostReachability = null;
if (pathMonitor != null)
{
pathMonitor.SnapshotHandler = null;
pathUpdateHandler = null;
pathMonitor.Cancel();
pathMonitor.Dispose();
pathMonitor = null;
}

#if !(MACCATALYST || MACOS)
#pragma warning disable CA1416 // Analyzer bug https://github.com/dotnet/roslyn-analyzers/issues/5938
Expand All @@ -191,14 +181,5 @@ void OnRestrictedStateChanged(CTCellularDataRestrictedState state)
}
#pragma warning restore BI1234
#endif

async void OnChange(NetworkReachabilityFlags flags)
{
// Add in artifical delay so the connection status has time to change
// else it will return true no matter what.
await Task.Delay(100);

ReachabilityChanged?.Invoke();
}
}
}
Loading