Fixed world being disposed multiple times (#952)

* fixing errors when stopping  editor application

* changelog

* pr feedback

* adding token cancellation

* more feedback

* slightly refactoring token lifecycle

* using block

Author: jessicafalk

CHANGELOG.md

@@ -6,6 +6,10 @@
 
 - Upgraded the project to be compatible with `2019.1.3f1`.
 
+### Fixed
+
+- Fixed a bug where a worker's `World` could get disposed multiple times if you stopped the application inside the Editor while the worker is being created.
+
 ## `0.2.2` - 2019-05-15
 
 ### Breaking Changes

workers/unity/Packages/com.improbable.gdk.core/Utility/TaskUtility.cs

@@ -0,0 +1,23 @@
+using System;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Improbable.Gdk.Core
+{
+    public static class TaskUtility
+    {
+        public static async Task<T> WithCancellation<T>(this Task<T> task, CancellationToken cancellationToken)
+        {
+            var tcs = new TaskCompletionSource<bool>();
+            using (cancellationToken.Register(s => ((TaskCompletionSource<bool>) s).TrySetResult(true), tcs))
+            {
+                if (task != await Task.WhenAny(task, tcs.Task))
+                {
+                    throw new OperationCanceledException(cancellationToken);
+                }
+            }
+
+            return task.Result;
+        }
+    }
+}

workers/unity/Packages/com.improbable.gdk.core/Utility/TaskUtility.cs.meta

@@ -1,4 +1,5 @@
 using System;
+using System.Threading;
 using System.Threading.Tasks;
 using Improbable.Gdk.ReactiveComponents;
 using Improbable.Worker.CInterop;
@@ -96,11 +97,13 @@ private static async Task<Worker> TryToConnectAsync(Future<Connection> connectio
             ILogDispatcher logger,
             Vector3 origin)
         {
-            var connection = await Task.Run(() => connectionFuture.Get());
-            if (connection.GetConnectionStatusCode() != ConnectionStatusCode.Success)
+            Connection connection;
+            using (var tokenSource = new CancellationTokenSource())
             {
-                throw new ConnectionFailedException(GetConnectionFailureReason(connection),
-                    ConnectionErrorReason.CannotEstablishConnection);
+                Action cancelTask = delegate { tokenSource?.Cancel(); };
+                Application.quitting += cancelTask;
+                connection = await Task.Run(() => connectionFuture.Get()).WithCancellation(tokenSource.Token);
+                Application.quitting -= cancelTask;
             }
 
             // A check is needed for the case that play mode is exited before the connection can complete.
@@ -111,6 +114,12 @@ private static async Task<Worker> TryToConnectAsync(Future<Connection> connectio
                     ConnectionErrorReason.EditorApplicationStopped);
             }
 
+            if (connection.GetConnectionStatusCode() != ConnectionStatusCode.Success)
+            {
+                throw new ConnectionFailedException(GetConnectionFailureReason(connection),
+                    ConnectionErrorReason.CannotEstablishConnection);
+            }
+
             var worker = new Worker(workerType, connection, logger, origin);
             logger.HandleLog(LogType.Log, new LogEvent("Successfully created a worker")
                 .WithField("WorkerId", worker.WorkerId));
@@ -121,7 +130,7 @@ private static async Task<Worker> TryToConnectAsync(Future<Connection> connectio
         ///     Connects to the SpatialOS Runtime via the Receptionist service and creates a <see cref="Worker" /> object
         ///     asynchronously.
         /// </summary>
-        /// <param name="config">
+        /// <param name="parameters">
         ///     The <see cref="ReceptionistConfig" /> object stores the configuration needed to connect via the
         ///     Receptionist Service.
         /// </param>
@@ -133,12 +142,13 @@ private static async Task<Worker> TryToConnectAsync(Future<Connection> connectio
         ///     <see cref="Worker" /> object upon connecting successfully.
         /// </returns>
         public static async Task<Worker> CreateWorkerAsync(
-            ReceptionistConfig config,
+            ReceptionistConfig parameters,
             ConnectionParameters connectionParameters,
-            ILogDispatcher logger, Vector3 origin)
+            ILogDispatcher logger,
+            Vector3 origin)
         {
             using (var connectionFuture =
-                Connection.ConnectAsync(config.ReceptionistHost, config.ReceptionistPort, config.WorkerId,
+                Connection.ConnectAsync(parameters.ReceptionistHost, parameters.ReceptionistPort, parameters.WorkerId,
                     connectionParameters))
             {
                 return await TryToConnectAsync(connectionFuture, connectionParameters.WorkerType, logger, origin);
@@ -149,7 +159,7 @@ public static async Task<Worker> CreateWorkerAsync(
         ///     Connects to the SpatialOS Runtime via the Locator service and creates a <see cref="Worker" /> object
         ///     asynchronously.
         /// </summary>
-        /// <param name="config">
+        /// <param name="parameters">
         ///     The <see cref="LocatorConfig" /> object stores the configuration needed to connect via the
         ///     Receptionist Service.
         /// </param>
@@ -163,7 +173,8 @@ public static async Task<Worker> CreateWorkerAsync(
         public static async Task<Worker> CreateWorkerAsync(
             LocatorConfig parameters,
             ConnectionParameters connectionParameters,
-            ILogDispatcher logger, Vector3 origin)
+            ILogDispatcher logger,
+            Vector3 origin)
         {
             using (var locator = new Locator(parameters.LocatorHost, parameters.LocatorParameters))
             {
@@ -187,7 +198,7 @@ public static async Task<Worker> CreateWorkerAsync(
         ///     Connects to the SpatialOS Runtime via the Alpha Locator service and creates a <see cref="Worker" /> object
         ///     asynchronously.
         /// </summary>
-        /// <param name="config">
+        /// <param name="parameters">
         ///     The <see cref="AlphaLocatorConfig" /> object stores the configuration needed to connect via the
         ///     Receptionist Service.
         /// </param>
@@ -201,7 +212,8 @@ public static async Task<Worker> CreateWorkerAsync(
         public static async Task<Worker> CreateWorkerAsync(
             AlphaLocatorConfig parameters,
             ConnectionParameters connectionParameters,
-            ILogDispatcher logger, Vector3 origin)
+            ILogDispatcher logger,
+            Vector3 origin)
         {
             using (var locator = new AlphaLocator(parameters.LocatorHost, parameters.LocatorParameters))
             {
@@ -268,7 +280,11 @@ private void AddCoreSystems()
 
         public void Dispose()
         {
-            World?.Dispose();
+            if (World != null && World.IsCreated)
+            {
+                World.Dispose();
+            }
+
             World = null;
             LogDispatcher?.Dispose();
             LogDispatcher = null;

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

@@ -8,6 +8,7 @@
 using Improbable.Worker.CInterop.Alpha;
 using Unity.Entities;
 using UnityEngine;
+using UnityEngine.EventSystems;
 
 namespace Improbable.Gdk.Core
 {
@@ -118,6 +119,8 @@ await Worker.CreateWorkerAsync(GetAlphaLocatorConfig(workerType), connectionPara
                 if (!Application.isPlaying)
                 {
                     Dispose();
+                    throw new ConnectionFailedException("Editor application stopped",
+                        ConnectionErrorReason.EditorApplicationStopped);
                 }
 
                 HandleWorkerConnectionEstablished();
@@ -368,6 +371,11 @@ private static async Task<Worker> ConnectWithRetries(ConnectionDelegate connecti
                 }
                 catch (ConnectionFailedException e)
                 {
+                    if (e.Reason == ConnectionErrorReason.EditorApplicationStopped)
+                    {
+                        throw;
+                    }
+
                     --remainingAttempts;
                     logger.HandleLog(LogType.Error,
                         new LogEvent($"Failed attempt {maxAttempts - remainingAttempts} to create worker")
@@ -405,7 +413,7 @@ public virtual void Dispose()
         {
             Worker?.Dispose();
             Worker = null;
-            Destroy(this);
+            UnityObjectDestroyer.Destroy(this);
         }
     }
 }