PR comments

Author: deansheather

Coder.Desktop.sln.DotSettings

@@ -30,7 +30,7 @@
         </HasMember>
       </And>
     </TypePattern.Match>
-    
+
     <Entry DisplayName="Fields">
       <Entry.Match>
         <And>
@@ -144,10 +144,6 @@
           <Kind Is="Delegate" />
         </And>
       </Entry.Match>
-
-      <Entry.SortBy>
-        <Name />
-      </Entry.SortBy>
     </Entry>
 
     <Entry DisplayName="Public Enums" Priority="100">
@@ -157,10 +153,6 @@
           <Kind Is="Enum" />
         </And>
       </Entry.Match>
-
-      <Entry.SortBy>
-        <Name />
-      </Entry.SortBy>
     </Entry>
 
     <Entry DisplayName="Static Fields and Constants">
@@ -193,21 +185,12 @@
           </Not>
         </And>
       </Entry.Match>
-
-      <Entry.SortBy>
-        <Readonly />
-        <Name />
-      </Entry.SortBy>
     </Entry>
 
     <Entry DisplayName="Events">
       <Entry.Match>
         <Kind Is="Event" />
       </Entry.Match>
-
-      <Entry.SortBy>
-        <Name />
-      </Entry.SortBy>
     </Entry>
 
     <Entry DisplayName="Constructors">
@@ -256,4 +239,4 @@
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=codervpn/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=hkey/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=replyable/@EntryIndexedValue">True</s:Boolean>
-	<s:Boolean x:Key="/Default/UserDictionary/Words/=serdes/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=serdes/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>

Vpn.Service/Manager.cs

@@ -1,5 +1,6 @@
 using System.Runtime.InteropServices;
 using Coder.Desktop.Vpn.Proto;
+using Coder.Desktop.Vpn.Utilities;
 using CoderSdk;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
@@ -27,6 +28,10 @@ public class Manager : IManager
     private readonly IDownloader _downloader;
     private readonly ILogger<Manager> _logger;
     private readonly ITunnelSupervisor _tunnelSupervisor;
+
+    // TunnelSupervisor already has protections against concurrent operations,
+    // but all the other stuff before starting the tunnel does not.
+    private readonly RaiiSemaphoreSlim _tunnelOperationLock = new(1, 1);
     private SemVersion? _lastServerVersion;
     private StartRequest? _lastStartRequest;
 
@@ -58,10 +63,18 @@ public async Task HandleClientRpcMessage(ReplyableRpcMessage<ServiceMessage, Cli
         {
             case ClientMessage.MsgOneofCase.Start:
                 // TODO: these sub-methods should be managed by some Task list and cancelled/awaited on stop
-                await HandleClientMessageStart(message, ct);
+                var startResponse = await HandleClientMessageStart(message.Message, ct);
+                await message.SendReply(new ServiceMessage
+                {
+                    Start = startResponse,
+                }, ct);
                 break;
             case ClientMessage.MsgOneofCase.Stop:
-                await HandleClientMessageStop(message, ct);
+                var stopResponse = await HandleClientMessageStop(message.Message, ct);
+                await message.SendReply(new ServiceMessage
+                {
+                    Stop = stopResponse,
+                }, ct);
                 break;
             case ClientMessage.MsgOneofCase.None:
             default:
@@ -75,92 +88,105 @@ public async Task StopAsync(CancellationToken ct = default)
         await _tunnelSupervisor.StopAsync(ct);
     }
 
-    private async Task HandleClientMessageStart(ReplyableRpcMessage<ServiceMessage, ClientMessage> message,
+    private async ValueTask<StartResponse> HandleClientMessageStart(ClientMessage message,
         CancellationToken ct)
     {
-        try
+        var opLock = await _tunnelOperationLock.LockAsync(TimeSpan.FromMilliseconds(500), ct);
+        if (opLock == null)
         {
-            var serverVersion =
-                await CheckServerVersionAndCredentials(message.Message.Start.CoderUrl, message.Message.Start.ApiToken,
-                    ct);
-            if (_tunnelSupervisor.IsRunning && _lastStartRequest != null &&
-                _lastStartRequest.Equals(message.Message.Start) && _lastServerVersion == serverVersion)
+            _logger.LogWarning("ClientMessage.Start: Tunnel operation lock timed out");
+            return new StartResponse
             {
-                // The client is requesting to start an identical tunnel while
-                // we're already running it.
-                _logger.LogInformation("ClientMessage.Start: Ignoring duplicate start request");
-                await message.SendReply(new ServiceMessage
+                Success = false,
+                ErrorMessage = "Could not acquire tunnel operation lock, another operation is in progress",
+            };
+        }
+
+        using (opLock)
+        {
+            try
+            {
+                var serverVersion =
+                    await CheckServerVersionAndCredentials(message.Start.CoderUrl, message.Start.ApiToken,
+                        ct);
+                if (_tunnelSupervisor.IsRunning && _lastStartRequest != null &&
+                    _lastStartRequest.Equals(message.Start) && _lastServerVersion == serverVersion)
                 {
-                    Start = new StartResponse
+                    // The client is requesting to start an identical tunnel while
+                    // we're already running it.
+                    _logger.LogInformation("ClientMessage.Start: Ignoring duplicate start request");
+                    return new StartResponse
                     {
                         Success = true,
-                    },
-                }, ct);
-                return;
-            }
-
-            _lastStartRequest = message.Message.Start;
-            _lastServerVersion = serverVersion;
+                    };
+                }
 
-            // Stop the tunnel if it's running so we don't have to worry about
-            // permissions issues when replacing the binary.
-            await _tunnelSupervisor.StopAsync(ct);
-            await DownloadTunnelBinaryAsync(message.Message.Start.CoderUrl, serverVersion, ct);
-            await _tunnelSupervisor.StartAsync(_config.TunnelBinaryPath, HandleTunnelRpcMessage, HandleTunnelRpcError,
-                ct);
+                _lastStartRequest = message.Start;
+                _lastServerVersion = serverVersion;
 
-            var reply = await _tunnelSupervisor.SendRequestAwaitReply(new ManagerMessage
-            {
-                Start = message.Message.Start,
-            }, ct);
-            if (reply.MsgCase != TunnelMessage.MsgOneofCase.Start)
-                throw new InvalidOperationException("Tunnel did not reply with a Start response");
+                // TODO: each section of this operation needs a timeout
+                // Stop the tunnel if it's running so we don't have to worry about
+                // permissions issues when replacing the binary.
+                await _tunnelSupervisor.StopAsync(ct);
+                await DownloadTunnelBinaryAsync(message.Start.CoderUrl, serverVersion, ct);
+                await _tunnelSupervisor.StartAsync(_config.TunnelBinaryPath, HandleTunnelRpcMessage,
+                    HandleTunnelRpcError,
+                    ct);
 
-            await message.SendReply(new ServiceMessage
-            {
-                Start = reply.Start,
-            }, ct);
-        }
-        catch (Exception e)
-        {
-            _logger.LogWarning(e, "ClientMessage.Start: Failed to start VPN client");
-            await message.SendReply(new ServiceMessage
+                var reply = await _tunnelSupervisor.SendRequestAwaitReply(new ManagerMessage
+                {
+                    Start = message.Start,
+                }, ct);
+                if (reply.MsgCase != TunnelMessage.MsgOneofCase.Start)
+                    throw new InvalidOperationException("Tunnel did not reply with a Start response");
+                return reply.Start;
+            }
+            catch (Exception e)
             {
-                Start = new StartResponse
+                _logger.LogWarning(e, "ClientMessage.Start: Failed to start VPN client");
+                return new StartResponse
                 {
                     Success = false,
                     ErrorMessage = e.ToString(),
-                },
-            }, ct);
+                };
+            }
         }
     }
 
-    private async Task HandleClientMessageStop(ReplyableRpcMessage<ServiceMessage, ClientMessage> message,
+    private async ValueTask<StopResponse> HandleClientMessageStop(ClientMessage message,
         CancellationToken ct)
     {
-        try
+        var opLock = await _tunnelOperationLock.LockAsync(TimeSpan.FromMilliseconds(500), ct);
+        if (opLock == null)
         {
-            // This will handle sending the Stop message to the tunnel for us.
-            await _tunnelSupervisor.StopAsync(ct);
-            await message.SendReply(new ServiceMessage
+            _logger.LogWarning("ClientMessage.Stop: Tunnel operation lock timed out");
+            return new StopResponse
             {
-                Stop = new StopResponse
-                {
-                    Success = true,
-                },
-            }, ct);
+                Success = false,
+                ErrorMessage = "Could not acquire tunnel operation lock, another operation is in progress",
+            };
         }
-        catch (Exception e)
+
+        using (opLock)
         {
-            _logger.LogWarning(e, "ClientMessage.Stop: Failed to stop VPN client");
-            await message.SendReply(new ServiceMessage
+            try
             {
-                Stop = new StopResponse
+                // This will handle sending the Stop message to the tunnel for us.
+                await _tunnelSupervisor.StopAsync(ct);
+                return new StopResponse
+                {
+                    Success = true,
+                };
+            }
+            catch (Exception e)
+            {
+                _logger.LogWarning(e, "ClientMessage.Stop: Failed to stop VPN client");
+                return new StopResponse
                 {
                     Success = false,
-                    ErrorMessage = e.Message,
-                },
-            }, ct);
+                    ErrorMessage = e.ToString(),
+                };
+            }
         }
     }
 

Vpn.Service/ManagerRpcService.cs

@@ -144,13 +144,19 @@ private async Task HandleRpcMessageAsync(ReplyableRpcMessage<ServiceMessage, Cli
 
     public async Task BroadcastAsync(ServiceMessage message, CancellationToken ct)
     {
+        // Looping over a ConcurrentDictionary is exception-safe, but any items
+        // added or removed during the loop may or may not be included.
         foreach (var (clientId, client) in _activeClients)
             try
             {
                 var cts = CancellationTokenSource.CreateLinkedTokenSource(ct);
                 cts.CancelAfter(5 * 1000);
                 await client.Speaker.SendMessage(message, cts.Token);
             }
+            catch (ObjectDisposedException)
+            {
+                // The speaker was likely closed while we were iterating.
+            }
             catch (Exception e)
             {
                 _logger.LogWarning(e, "Failed to send message to client {ClientId}", clientId);

Vpn/Utilities/RaiiSemaphoreSlim.cs

@@ -24,6 +24,13 @@ public async ValueTask<IDisposable> LockAsync(CancellationToken ct = default)
         return new Lock(_semaphore);
     }
 
+    public async ValueTask<IDisposable?> LockAsync(TimeSpan timeout, CancellationToken ct = default)
+    {
+        if (await _semaphore.WaitAsync(timeout, ct)) return null;
+
+        return new Lock(_semaphore);
+    }
+
     private class Lock : IDisposable
     {
         private readonly SemaphoreSlim _semaphore1;