Async status host handlers to reduce ACZ stalling (#2235)

Robust.Server/ServerStatus/IStatusHost.cs

@@ -8,6 +8,7 @@ namespace Robust.Server.ServerStatus
         void Start();
 
         void AddHandler(StatusHostHandler handler);
+        void AddHandler(StatusHostHandlerAsync handler);
 
         /// <summary>
         ///     Invoked when a client queries a status request from the server.
@@ -25,4 +26,4 @@ namespace Robust.Server.ServerStatus
         /// </summary>
         event Action<JObject> OnInfoRequest;
     }
-}
+}

Robust.Server/ServerStatus/StatusHost.ClientZip.cs

@@ -1,4 +1,6 @@
 using System;
+using System.Threading;
+using System.Threading.Tasks;
 using System.Collections.Generic;
 using System.IO;
 using System.IO.Compression;
@@ -15,14 +17,13 @@ namespace Robust.Server.ServerStatus
     internal sealed partial class StatusHost
     {
         // Lock used while working on the ACZ.
-        private readonly object _aczLock = new();
+        private readonly SemaphoreSlim _aczLock = new(1, 1);
         // If an attempt has been made to prepare the ACZ.
         private bool _aczPrepareAttempted = false;
         // Automatic Client Zip
-        private byte[]? _aczData;
-        private string _aczHash = "";
+        private AutomaticClientZipInfo? _aczPrepared;
 
-        private bool HandleAutomaticClientZip(IStatusHandlerContext context)
+        private async Task<bool> HandleAutomaticClientZip(IStatusHandlerContext context)
         {
             if (!context.IsGetLike || context.Url!.AbsolutePath != "/client.zip")
             {
@@ -35,47 +36,59 @@ namespace Robust.Server.ServerStatus
                 return true;
             }
 
-            var result = PrepareACZ();
+            var result = await PrepareACZ();
             if (result == null)
             {
                 context.Respond("Automatic Client Zip was not preparable.", HttpStatusCode.InternalServerError);
                 return true;
             }
 
-            context.Respond(result, HttpStatusCode.OK, "application/zip");
+            context.Respond(result.Value.Data, HttpStatusCode.OK, "application/zip");
             return true;
         }
 
-        private byte[]? PrepareACZ()
+        // Only call this if the download URL is not available!
+        private async Task<AutomaticClientZipInfo?> PrepareACZ()
         {
-            lock (_aczLock)
+            // Take the ACZ lock asynchronously
+            await _aczLock.WaitAsync();
+            try
             {
-                if (_aczPrepareAttempted) return _aczData;
+                // Setting this now ensures that it won't fail repeatedly on exceptions/etc.
+                if (_aczPrepareAttempted) return _aczPrepared;
                 _aczPrepareAttempted = true;
+                // ACZ hasn't been prepared, prepare it
                 byte[] data;
                 try
                 {
-                    var maybeData = PrepareACZInnards();
+                    // Run actual ACZ generation via Task.Run because it's synchronous
+                    var maybeData = await Task.Run(PrepareACZInnards);
                     if (maybeData == null)
                     {
+                        _httpSawmill.Error("StatusHost PrepareACZ failed (server will not be usable from launcher!)");
                         return null;
                     }
                     data = maybeData;
                 }
                 catch (Exception e)
                 {
-                    _httpSawmill.Error($"Exception in StatusHost PrepareACZ: {e}");
+                    _httpSawmill.Error($"Exception in StatusHost PrepareACZ (server will not be usable from launcher!): {e}");
                     return null;
                 }
-                _aczData = data;
-                using var sha = SHA256.Create();
-                _aczHash = Convert.ToHexString(sha.ComputeHash(data));
-                return data;
+                _aczPrepared = new AutomaticClientZipInfo(data);
+                return _aczPrepared;
+            }
+            finally
+            {
+                _aczLock.Release();
             }
         }
 
+        // -- All methods from this point forward do not access the ACZ global state --
+
         private byte[]? PrepareACZInnards()
         {
+            // All of these should Info on success and Error on null-return failure
             return PrepareACZViaFile() ?? PrepareACZViaMagic();
         }
 
@@ -83,6 +96,7 @@ namespace Robust.Server.ServerStatus
         {
             var path = PathHelpers.ExecutableRelativeFile("Content.Client.zip");
             if (!File.Exists(path)) return null;
+            _httpSawmill.Info($"StatusHost found client zip: {path}");
             return File.ReadAllBytes(path);
         }
 
@@ -124,8 +138,21 @@ namespace Robust.Server.ServerStatus
                 }
             }
             archive.Dispose();
+            _httpSawmill.Info($"StatusHost synthesized client zip!");
             return outStream.ToArray();
         }
     }
 
+    internal struct AutomaticClientZipInfo
+    {
+        public readonly byte[] Data;
+        public readonly string Hash;
+
+        public AutomaticClientZipInfo(byte[] data)
+        {
+            Data = data;
+            using var sha = SHA256.Create();
+            Hash = Convert.ToHexString(sha.ComputeHash(data));
+        }
+    }
 }

Robust.Server/ServerStatus/StatusHost.Handlers.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Threading.Tasks;
 using System.Net;
 using Newtonsoft.Json.Linq;
 using Robust.Shared;
@@ -50,7 +51,7 @@ namespace Robust.Server.ServerStatus
             return true;
         }
 
-        private bool HandleInfo(IStatusHandlerContext context)
+        private async Task<bool> HandleInfo(IStatusHandlerContext context)
         {
             if (!context.IsGetLike || context.Url!.AbsolutePath != "/info")
             {
@@ -63,7 +64,7 @@ namespace Robust.Server.ServerStatus
 
             if (string.IsNullOrEmpty(downloadUrl))
             {
-                buildInfo = PrepareACZBuildInfo();
+                buildInfo = await PrepareACZBuildInfo();
             }
             else
             {
@@ -105,12 +106,10 @@ namespace Robust.Server.ServerStatus
             return true;
         }
 
-        private JObject? PrepareACZBuildInfo()
+        private async Task<JObject?> PrepareACZBuildInfo()
         {
-            if (PrepareACZ() == null)
-            {
-                return null;
-            }
+            var acz = await PrepareACZ();
+            if (acz == null) return null;
 
             // Automatic - pass to ACZ
             // Unfortunately, we still can't divine engine version.
@@ -126,10 +125,10 @@ namespace Robust.Server.ServerStatus
             {
                 ["engine_version"] = engineVersion,
                 ["fork_id"] = fork,
-                ["version"] = _aczHash,
+                ["version"] = acz.Value.Hash,
                 // Don't supply a download URL - like supplying an empty self-address
                 ["download_url"] = "",
-                ["hash"] = _aczHash,
+                ["hash"] = acz.Value.Hash,
             };
         }
     }

Robust.Server/ServerStatus/StatusHost.cs

@@ -37,14 +37,14 @@ namespace Robust.Server.ServerStatus
         [Dependency] private readonly IPlayerManager _playerManager = default!;
 
         private static readonly JsonSerializer JsonSerializer = new();
-        private readonly List<StatusHostHandler> _handlers = new();
+        private readonly List<StatusHostHandlerAsync> _handlers = new();
         private HttpListener? _listener;
         private TaskCompletionSource? _stopSource;
         private ISawmill _httpSawmill = default!;
 
         private string? _serverNameCache;
 
-        public Task ProcessRequestAsync(HttpListenerContext context)
+        public async Task ProcessRequestAsync(HttpListenerContext context)
         {
             var apiContext = (IStatusHandlerContext) new ContextImpl(context);
 
@@ -55,9 +55,9 @@ namespace Robust.Server.ServerStatus
             {
                 foreach (var handler in _handlers)
                 {
-                    if (handler(apiContext))
+                    if (await handler(apiContext))
                     {
-                        return Task.CompletedTask;
+                        return;
                     }
                 }
 
@@ -75,8 +75,6 @@ namespace Robust.Server.ServerStatus
             _httpSawmill.Debug(Sawmill, $"{method} {context.Request.Url!.PathAndQuery} {context.Response.StatusCode} " +
                                          $"{(HttpStatusCode) context.Response.StatusCode} to {context.Request.RemoteEndPoint}");
                                          */
-
-            return Task.CompletedTask;
         }
 
         public event Action<JObject>? OnStatusRequest;
@@ -84,6 +82,11 @@ namespace Robust.Server.ServerStatus
         public event Action<JObject>? OnInfoRequest;
 
         public void AddHandler(StatusHostHandler handler)
+        {
+            _handlers.Add((ctx) => Task.FromResult(handler(ctx)));
+        }
+
+        public void AddHandler(StatusHostHandlerAsync handler)
         {
             _handlers.Add(handler);
         }

Robust.Server/ServerStatus/StatusHostHandler.cs

@@ -1,5 +1,9 @@
+using System.Threading.Tasks;
+
 namespace Robust.Server.ServerStatus
 {
     public delegate bool StatusHostHandler(
         IStatusHandlerContext context);
-}
+    public delegate Task<bool> StatusHostHandlerAsync(
+        IStatusHandlerContext context);
+}