Change up worker connector semantics slightly (#1365)

Author: Jamie Brynes

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+### Breaking Changes
+
+- `WorkerConnector.HandleWorkerConnectionFailure` has been removed and `WorkerConnector.Connect` now throws exceptions for connection errors instead. [#1365](https://github.com/spatialos/gdk-for-unity/pull/1365)
+- `WorkerConnector` no longer destroys itself in `Dispose`. [#1365](https://github.com/spatialos/gdk-for-unity/pull/1365)
+
 ### Added
 
 - Added the ability to select a specific cluster for deployments in the [Deployment Launcher](https://documentation.improbable.io/gdk-for-unity/docs/deployment-launcher). [#1357](https://github.com/spatialos/gdk-for-unity/pull/1357)

UPGRADE_GUIDE.md

@@ -1,5 +1,55 @@
 # Upgrade Guide
 
+## From `0.3.5` to `0.3.6`
+
+### WorkerConnector changes
+
+There are two related changes that you will need to adjust for.
+
+The `WorkerConnector.Connect` class would previously trigger the `HandleWorkerConnectionFailure` callback if the connection failed. We've now removed this callback, and `WorkerConnector.Connect` will now throw an exception for a failed connection.
+
+For example:
+
+```csharp
+public class MyWorkerConnector : WorkerConnector
+{
+    public async void Start()
+    {
+        // Setup connection flow.
+        var builder = ...;
+
+        await Connect(builder, new ForwardingDispatcher());
+    }
+
+    protected override void HandleWorkerConnectionFailure(string errorMessage)
+    {
+        Debug.LogError(errorMessage);
+    }
+}
+```
+
+Would change to:
+
+```csharp
+public class MyWorkerConnector : WorkerConnector
+{
+    public async void Start()
+    {
+        // Setup connection flow.
+        var builder = ...;
+
+        try
+        {
+            await Connect(builder, new ForwardingDispatcher());
+        }
+        catch (Exception e)
+        {
+            Debug.LogException(e);
+        }
+    }
+}
+```
+
 ## From `0.3.4` to `0.3.5`
 
 ### Unity 2019.3 upgrade

workers/unity/Assets/Playground/Scripts/Worker/ClientWorkerConnector.cs

@@ -43,12 +43,8 @@ private async void Start()
                 builder.SetConnectionFlow(new ReceptionistFlow(CreateNewWorkerId(WorkerUtils.UnityClient)));
             }
 
-            await Connect(builder, new ForwardingDispatcher()).ConfigureAwait(false);
-        }
+            await Connect(builder, new ForwardingDispatcher());
 
-        protected override void HandleWorkerConnectionEstablished()
-        {
-            WorkerUtils.AddClientSystems(Worker.World);
             if (level == null)
             {
                 return;
@@ -57,6 +53,11 @@ protected override void HandleWorkerConnectionEstablished()
             levelInstance = Instantiate(level, transform.position, transform.rotation);
         }
 
+        protected override void HandleWorkerConnectionEstablished()
+        {
+            WorkerUtils.AddClientSystems(Worker.World);
+        }
+
         public override void Dispose()
         {
             if (levelInstance != null)

workers/unity/Assets/Playground/Scripts/Worker/GameLogicWorkerConnector.cs

@@ -37,12 +37,7 @@ private async void Start()
                 .SetConnectionFlow(flow)
                 .SetConnectionParameters(connectionParameters);
 
-            await Connect(builder, new ForwardingDispatcher()).ConfigureAwait(false);
-        }
-
-        protected override void HandleWorkerConnectionEstablished()
-        {
-            WorkerUtils.AddGameLogicSystems(Worker.World);
+            await Connect(builder, new ForwardingDispatcher());
 
             if (level == null)
             {
@@ -52,6 +47,11 @@ protected override void HandleWorkerConnectionEstablished()
             levelInstance = Instantiate(level, transform.position, transform.rotation);
         }
 
+        protected override void HandleWorkerConnectionEstablished()
+        {
+            WorkerUtils.AddGameLogicSystems(Worker.World);
+        }
+
         public override void Dispose()
         {
             if (levelInstance != null)

workers/unity/Assets/Playground/Scripts/Worker/MobileClientWorkerConnector.cs

@@ -39,12 +39,7 @@ public async void Start()
                     throw new ArgumentException("Received unsupported connection service.");
             }
 
-            await Connect(builder, new ForwardingDispatcher()).ConfigureAwait(false);
-        }
-
-        protected override void HandleWorkerConnectionEstablished()
-        {
-            WorkerUtils.AddClientSystems(Worker.World);
+            await Connect(builder, new ForwardingDispatcher());
 
             if (level == null)
             {
@@ -54,6 +49,11 @@ protected override void HandleWorkerConnectionEstablished()
             levelInstance = Instantiate(level, transform.position, transform.rotation);
         }
 
+        protected override void HandleWorkerConnectionEstablished()
+        {
+            WorkerUtils.AddClientSystems(Worker.World);
+        }
+
         public override void Dispose()
         {
             if (levelInstance != null)

workers/unity/Packages/io.improbable.gdk.core/Worker/WorkerConnector.cs

@@ -107,13 +107,8 @@ protected async Task Connect(IConnectionHandlerBuilder builder, ILogDispatcher l
                 PlayerLoopUtils.ResolveSystemGroups(Worker.World);
                 ScriptBehaviourUpdateOrder.UpdatePlayerLoop(Worker.World, PlayerLoop.GetCurrentPlayerLoop());
             }
-            catch (Exception e)
+            catch (Exception)
             {
-                logger.HandleLog(LogType.Error, new LogEvent("Failed to create worker")
-                    .WithException(e)
-                    .WithField("WorkerType", builder.WorkerType)
-                    .WithField("Message", e.Message)
-                    .WithField("Stacktrace", e.StackTrace));
 #if UNITY_EDITOR
                 // Temporary warning to be replaced when we can reliably detect if a local runtime is running, or not.
                 logger.HandleLog(LogType.Warning,
@@ -124,9 +119,10 @@ protected async Task Connect(IConnectionHandlerBuilder builder, ILogDispatcher l
                 // A check is needed for the case that play mode is exited before the connection can complete.
                 if (Application.isPlaying)
                 {
-                    HandleWorkerConnectionFailure(e.Message);
                     Dispose();
                 }
+
+                throw;
             }
             finally
             {
@@ -155,10 +151,6 @@ protected virtual void HandleWorkerConnectionEstablished()
         {
         }
 
-        protected virtual void HandleWorkerConnectionFailure(string errorMessage)
-        {
-        }
-
         private static async Task<WorkerInWorld> ConnectWithRetries(IConnectionHandlerBuilder connectionHandlerBuilder, int maxAttempts,
             ILogDispatcher logger, string workerType, Vector3 origin)
         {
@@ -251,7 +243,6 @@ public virtual void Dispose()
             RemoveFromPlayerLoop();
             Worker?.Dispose();
             Worker = null;
-            UnityObjectDestroyer.Destroy(this);
         }
 
         private void RemoveFromPlayerLoop()