Makes the update process async

This has a pro and a con:

Pro: No more HTTP timeouts waiting for the package to download
Con: No API endpoint for update errors

This is a worthwhile trade.

Also adds a bunch of logging to Server and handles restart handler exceptions

src/Tgstation.Server.Host/Controllers/AdministrationController.cs

@@ -150,12 +150,19 @@ namespace Tgstation.Server.Host.Controllers
 			if (model.NewVersion.Major != application.Version.Major)
 				return BadRequest(new ErrorMessage { Message = "Cannot update to a different suite version!" });
 
-			Logger.LogInformation("Updating to version {0}...", model.NewVersion);
+			if(!serverUpdater.WatchdogPresent)
+				return UnprocessableEntity(new ErrorMessage
+				{
+					Message = RestartNotSupportedException
+				});
+
+			Logger.LogDebug("Looking for GitHub releases version {0}...", model.NewVersion);
 			IEnumerable<Release> releases;
 			try
 			{
 				var gitHubClient = GetGitHubClient();
 				releases = (await gitHubClient.Repository.Release.GetAll(updatesConfiguration.GitHubRepositoryId).ConfigureAwait(false)).Where(x => x.TagName.StartsWith(updatesConfiguration.GitTagPrefix, StringComparison.InvariantCulture));
+				cancellationToken.ThrowIfCancellationRequested();
 			}
 			catch (RateLimitExceededException e)
 			{
@@ -174,24 +181,12 @@ namespace Tgstation.Server.Host.Controllers
 					var asset = release.Assets.Where(x => x.Name == updatesConfiguration.UpdatePackageAssetName).FirstOrDefault();
 					if (asset == default)
 						continue;
-
-					Logger.LogDebug("Downloading update package...");
-					var assetBytes = await ioManager.DownloadFile(new Uri(asset.BrowserDownloadUrl), cancellationToken).ConfigureAwait(false);
-					try
-					{
-						Logger.LogDebug("Extracting server update...");
-						if (!await serverUpdater.ApplyUpdate(version, assetBytes, ioManager, cancellationToken).ConfigureAwait(false))
-							return UnprocessableEntity(new ErrorMessage
-							{
-								Message = RestartNotSupportedException
-							}); //unprocessable entity
-					}
-					catch (InvalidOperationException)
-					{
-						Logger.LogDebug("Unable to find github release version!");
-						return StatusCode((int)HttpStatusCode.ServiceUnavailable);  //we were beat to the punch, really shouldn't happen but heat death of the universe and what not
-					}
-					Logger.LogInformation("Update staged! Restarting host...");
+					
+					if (!serverUpdater.ApplyUpdate(version, new Uri(asset.BrowserDownloadUrl), ioManager))
+						return Conflict(new ErrorMessage
+						{
+							Message = "An update operation is already in progress!"
+						});
 					return Accepted();  //gtfo of here before all the cancellation tokens fire
 				}
 
@@ -205,15 +200,16 @@ namespace Tgstation.Server.Host.Controllers
 		{
 			try
 			{
-				var result = await serverUpdater.Restart().ConfigureAwait(false);
-				if (result)
-					Logger.LogInformation("Restarting host by request...");
-				else
-					Logger.LogDebug("Restart request failed due to lack of host watchdog!");
-				return result ? (IActionResult)Ok() : UnprocessableEntity(new ErrorMessage
+				if (!serverUpdater.WatchdogPresent)
 				{
-					Message = RestartNotSupportedException
-				});
+					Logger.LogDebug("Restart request failed due to lack of host watchdog!");
+					return UnprocessableEntity(new ErrorMessage
+					{
+						Message = RestartNotSupportedException
+					});
+				}
+				await serverUpdater.Restart().ConfigureAwait(false);
+				return Ok();
 			}
 			catch (InvalidOperationException)
 			{

src/Tgstation.Server.Host/Core/Application.cs

@@ -317,7 +317,8 @@ namespace Tgstation.Server.Host.Core
 			logger.LogInformation(VersionString);
 
 			//attempt to restart the server if the configuration changes
-			ChangeToken.OnChange(configuration.GetReloadToken, () => serverControl.Restart());
+			if(serverControl.WatchdogPresent)
+				ChangeToken.OnChange(configuration.GetReloadToken, () => serverControl.Restart());
 
 			//now setup the HTTP request pipeline
 

src/Tgstation.Server.Host/Core/IServerControl.cs

@@ -10,15 +10,19 @@ namespace Tgstation.Server.Host.Core
 	/// </summary>
 	public interface IServerControl
 	{
+		/// <summary>
+		/// <see langword="true"/> if live updates are supported, <see langword="false"/>. <see cref="ApplyUpdate(Version, Uri, IIOManager, CancellationToken)"/> and <see cref="Restart"/> will fail if this is <see langword="false"/>
+		/// </summary>
+		bool WatchdogPresent { get; }
+
 		/// <summary>
 		/// Run a new <see cref="Host"/> assembly and stop the current one. This will likely trigger all active <see cref="CancellationToken"/>s
 		/// </summary>
 		/// <param name="version">The <see cref="Version"/> the <see cref="IServerControl"/> is updating to</param>
-		/// <param name="updateZipData">The <see cref="byte"/>s of the .zip file that contains the new <see cref="Host"/> assembly</param>
+		/// <param name="updateZipUrl">The <see cref="Uri"/> that points to the .zip file that contains the new <see cref="Host"/> assembly</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation</param>
-		/// <param name="ioManager">The <see cref="IIOManager"/> for the operation</param>
-		/// <returns>A <see cref="Task{TResult}"/> resulting in <see langword="true"/> if live updates are supported, <see langword="false"/> otherwise</returns>
-		Task<bool> ApplyUpdate(Version version, byte[] updateZipData, IIOManager ioManager, CancellationToken cancellationToken);
+		/// <returns><see langword="true"/> if the update started successfully, <see langword="false"/> if there was another update in progress</returns>
+		bool ApplyUpdate(Version version, Uri updateZipUrl, IIOManager ioManager);
 
 		/// <summary>
 		/// Register a given <paramref name="handler"/> to run before stopping the server for a restart
@@ -30,7 +34,7 @@ namespace Tgstation.Server.Host.Core
 		/// <summary>
 		/// Restarts the <see cref="Host"/>
 		/// </summary>
-		/// <returns>A <see cref="Task{TResult}"/> resulting in <see langword="true"/> if live restarts are supported, <see langword="false"/> otherwise</returns>
-		Task<bool> Restart();
+		/// <returns>A <see cref="Task"/> representing the running operation</returns>
+		Task Restart();
 	}
 }

src/Tgstation.Server.Host/IServer.cs

@@ -1,5 +1,4 @@
-using System;
-using System.Threading;
+using System.Threading;
 using System.Threading.Tasks;
 
 namespace Tgstation.Server.Host
@@ -7,7 +6,7 @@ namespace Tgstation.Server.Host
 	/// <summary>
 	/// Represents the host
 	/// </summary>
-	public interface IServer : IDisposable
+	public interface IServer
 	{
 		/// <summary>
 		/// If the <see cref="IServer"/> should restart

src/Tgstation.Server.Host/Program.cs

@@ -38,32 +38,30 @@ namespace Tgstation.Server.Host
 				updatePath = null;
 			try
 			{
-				using (var server = serverFactory.CreateServer(listArgs.ToArray(), updatePath))
+				var server = serverFactory.CreateServer(listArgs.ToArray(), updatePath);
+				try
 				{
-					try
+					using (var cts = new CancellationTokenSource())
 					{
-						using (var cts = new CancellationTokenSource())
+						void AppDomainHandler(object a, EventArgs b) => cts.Cancel();
+						AppDomain.CurrentDomain.ProcessExit += AppDomainHandler;
+						try
 						{
-							void AppDomainHandler(object a, EventArgs b) => cts.Cancel();
-							AppDomain.CurrentDomain.ProcessExit += AppDomainHandler;
-							try
+							Console.CancelKeyPress += (a, b) =>
 							{
-								Console.CancelKeyPress += (a, b) =>
-								{
-									b.Cancel = true;
-									cts.Cancel();
-								};
-								await server.RunAsync(cts.Token).ConfigureAwait(false);
-							}
-							finally
-							{
-								AppDomain.CurrentDomain.ProcessExit -= AppDomainHandler;
-							}
+								b.Cancel = true;
+								cts.Cancel();
+							};
+							await server.RunAsync(cts.Token).ConfigureAwait(false);
+						}
+						finally
+						{
+							AppDomain.CurrentDomain.ProcessExit -= AppDomainHandler;
 						}
 					}
-					catch (OperationCanceledException) { }
-					return server.RestartRequested ? 1 : 0;
 				}
+				catch (OperationCanceledException) { }
+				return server.RestartRequested ? 1 : 0;
 			}
 			catch (Exception e)
 			{

src/Tgstation.Server.Host/Server.cs

@@ -1,5 +1,6 @@
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -12,41 +13,46 @@ using Tgstation.Server.Host.IO;
 namespace Tgstation.Server.Host
 {
 	/// <inheritdoc />
+#pragma warning disable CA1001 // Types that own disposable fields should be disposable
 	sealed class Server : IServer, IServerControl
+#pragma warning restore CA1001 // Types that own disposable fields should be disposable
 	{
 		/// <inheritdoc />
 		public bool RestartRequested { get; private set; }
 
+		/// <inheritdoc />
+		public bool WatchdogPresent => updatePath != null;
+
 		/// <summary>
 		/// The <see cref="IWebHostBuilder"/> for the <see cref="Server"/>
 		/// </summary>
 		readonly IWebHostBuilder webHostBuilder;
 
-		/// <summary>
-		/// The <see cref="SemaphoreSlim"/> for the <see cref="Server"/>
-		/// </summary>
-		readonly SemaphoreSlim semaphore;
-
-		/// <summary>
-		/// The absolute path to install updates to
-		/// </summary>
-		readonly string updatePath;
-		
 		/// <summary>
 		/// The <see cref="IRestartHandler"/>s to run when the <see cref="Server"/> restarts
 		/// </summary>
 		readonly List<IRestartHandler> restartHandlers;
 
 		/// <summary>
-		/// If a server update has been applied
+		/// The absolute path to install updates to
 		/// </summary>
-		bool updated;
+		readonly string updatePath;
+
+		/// <summary>
+		/// The <see cref="ILogger"/> for the <see cref="Server"/>
+		/// </summary>
+		ILogger<Server> logger;
 
 		/// <summary>
 		/// The <see cref="cancellationTokenSource"/> for the <see cref="Server"/>
 		/// </summary>
 		CancellationTokenSource cancellationTokenSource;
 
+		/// <summary>
+		/// If a server update has been or is being applied
+		/// </summary>
+		bool updating;
+
 		/// <summary>
 		/// Construct a <see cref="Server"/>
 		/// </summary>
@@ -58,13 +64,20 @@ namespace Tgstation.Server.Host
 			this.updatePath = updatePath;
 
 			restartHandlers = new List<IRestartHandler>();
-			semaphore = new SemaphoreSlim(1);
-			updated = false;
-			RestartRequested = false;
 		}
 
-		/// <inheritdoc />
-		public void Dispose() => semaphore.Dispose();
+		/// <summary>
+		/// Throws an <see cref="InvalidOperationException"/> if the <see cref="IServerControl"/> cannot be used
+		/// </summary>
+		/// <param name="checkWatchdog">If <see cref="WatchdogPresent"/> should be checked</param>
+		void CheckSanity(bool checkWatchdog)
+		{
+			if (checkWatchdog && !WatchdogPresent)
+				throw new InvalidOperationException("Server restarts are not supported");
+
+			if (cancellationTokenSource == null || logger == null)
+				throw new InvalidOperationException("Tried to control a non-running Server!");
+		}
 
 		/// <inheritdoc />
 		public async Task RunAsync(CancellationToken cancellationToken)
@@ -83,51 +96,91 @@ namespace Tgstation.Server.Host
 				}
 				using (var webHost = webHostBuilder
 					.UseStartup<Application>()
-					.ConfigureServices((serviceCollection) => serviceCollection.AddSingleton<IServerControl>(this))
+					.ConfigureServices(serviceCollection => serviceCollection.AddSingleton<IServerControl>(this))
 					.Build()
 				)
+				{
+					logger = webHost.Services.GetRequiredService<ILogger<Server>>();
 					await webHost.RunAsync(cancellationTokenSource.Token).ConfigureAwait(false);
+				}
 			}
 		}
 
 		/// <inheritdoc />
-		public async Task<bool> ApplyUpdate(Version version, byte[] updateZipData, IIOManager ioManager, CancellationToken cancellationToken)
+		public bool ApplyUpdate(Version version, Uri updateZipUrl, IIOManager ioManager)
 		{
 			if (version == null)
 				throw new ArgumentNullException(nameof(version));
-			if (updateZipData == null)
-				throw new ArgumentNullException(nameof(updateZipData));
+			if (updateZipUrl == null)
+				throw new ArgumentNullException(nameof(updateZipUrl));
 			if (ioManager == null)
 				throw new ArgumentNullException(nameof(ioManager));
 
-			if (updatePath == null)
-				return false;
-			using (await SemaphoreSlimContext.Lock(semaphore, cancellationToken).ConfigureAwait(false))
+			CheckSanity(true);
+
+			logger.LogTrace("Begin ApplyUpdate...");
+
+			lock (this)
+			{
+				if (updating || RestartRequested)
+				{
+					logger.LogTrace("Aborted due to concurrency conflict!");
+					return false;
+				}
+				updating = true;
+			}
+
+			async void RunUpdate()
 			{
-				if (updated)
-					throw new InvalidOperationException("ApplyUpdate has already been called!");
-				updated = true;
 				try
 				{
-					await ioManager.ZipToDirectory(updatePath, updateZipData, cancellationToken).ConfigureAwait(false);
+					logger.LogInformation("Updating server to version {0} ({1})...", version, updateZipUrl);
+
+					if (cancellationTokenSource == null)
+						throw new InvalidOperationException("Tried to update a non-running Server!");
+					var cancellationToken = cancellationTokenSource.Token;
+
+					logger.LogTrace("Downloading zip package...");
+					var updateZipData = await ioManager.DownloadFile(updateZipUrl, cancellationToken).ConfigureAwait(false);
+
+					try
+					{
+						logger.LogTrace("Exctracting zip package to {0}...", updatePath);
+						await ioManager.ZipToDirectory(updatePath, updateZipData, cancellationToken).ConfigureAwait(false);
+					}
+					catch (Exception e)
+					{
+						updating = false;
+						try
+						{
+							//important to not leave this directory around if possible
+							await ioManager.DeleteDirectory(updatePath, default).ConfigureAwait(false);
+						}
+						catch (Exception e2)
+						{
+							throw new AggregateException(e, e2);
+						}
+						throw;
+					}
+
+					await Restart(version).ConfigureAwait(false);
+				}
+				catch (OperationCanceledException)
+				{
+					logger.LogInformation("Server update cancelled!");
 				}
 				catch (Exception e)
 				{
-					updated = false;
-					try
-					{
-						//important to not leave this directory around if possible
-						await ioManager.DeleteDirectory(updatePath, default).ConfigureAwait(false);
-					}
-					catch (Exception e2)
-					{
-						throw new AggregateException(e, e2);
-					}
-					throw;
+					logger.LogError("Error updating server! Exception: {0}", e);
+				}
+				finally
+				{
+					updating = false;
 				}
-				await Restart(version).ConfigureAwait(false);
-				return true;
 			}
+
+			RunUpdate();
+			return true;
 		}
 
 		/// <inheritdoc />
@@ -135,16 +188,18 @@ namespace Tgstation.Server.Host
 		{
 			if (handler == null)
 				throw new ArgumentNullException(nameof(handler));
-			if (cancellationTokenSource == null)
-				throw new InvalidOperationException("Tried to register an update action on a non-running Server!");
+
+			CheckSanity(false);
+
 			lock (this)
 				if (!RestartRequested)
 				{
+					logger.LogTrace("Registering restart handler {0}...", handler);
 					restartHandlers.Add(handler);
 					return new RestartRegistration(() =>
 					{
 						lock (this)
-							if(!RestartRequested)
+							if (!RestartRequested)
 								restartHandlers.Remove(handler);
 					});
 				}
@@ -152,43 +207,54 @@ namespace Tgstation.Server.Host
 		}
 
 		/// <inheritdoc />
-		public Task<bool> Restart() => Restart(null);
+		public Task Restart() => Restart(null);
 
 		/// <summary>
 		/// Implements <see cref="Restart()"/>
 		/// </summary>
 		/// <param name="newVersion">The <see cref="Version"/> of any potential updates being applied</param>
 		/// <returns></returns>
-		async Task<bool> Restart(Version newVersion)
+		async Task Restart(Version newVersion)
 		{
-			if (updatePath == null)
-				return false;
-			if (cancellationTokenSource == null)
-				throw new InvalidOperationException("Tried to restart a non-running Server!");
+			CheckSanity(true);
+
+			logger.LogTrace("Begin Restart...");
+
 			lock (this)
 			{
-				if (RestartRequested)
-					return true;
+				if ((updating && newVersion == null) || RestartRequested)
+				{
+					logger.LogTrace("Aborted due to concurrency conflict!");
+					return;
+				}
 				RestartRequested = true;
 			}
 
+			logger.LogInformation("Restarting server...");
+
 			using (var cts = new CancellationTokenSource())
 			{
+				logger.LogTrace("Running restart handlers...");
 				var cancellationToken = cts.Token;
 				var eventsTask = Task.WhenAll(restartHandlers.Select(x => x.HandleRestart(newVersion, cancellationToken)).ToList());
 				//YA GOT 10 SECONDS
 				var expiryTask = Task.Delay(TimeSpan.FromSeconds(10));
 				await Task.WhenAny(eventsTask, expiryTask).ConfigureAwait(false);
+				logger.LogTrace("Joining restart handlers...");
 				cts.Cancel();
 				try
 				{
 					await eventsTask.ConfigureAwait(false);
 				}
 				catch (OperationCanceledException) { }
+				catch (Exception e)
+				{
+					logger.LogError("Restart handlers error! Exception: {0}", e);
+				}
 			}
 
+			logger.LogTrace("Stopping host...");
 			cancellationTokenSource.Cancel();
-			return true;
 		}
 	}
 }