fix(rhino-importer): Use main thread always for document creation (#1161)

* Use main thread

* format

* configure await false

* pass args only once

Author: JR-Morgan

Importers/Rhino/Speckle.Importers.Rhino/Internal/ImporterInstance.cs

@@ -1,4 +1,4 @@
-using System.Diagnostics.CodeAnalysis;
+using System.Diagnostics.Contracts;
 using Microsoft.Extensions.Logging;
 using Rhino;
 using Rhino.Runtime.InProcess;
@@ -9,55 +9,34 @@
 
 namespace Speckle.Importers.Rhino.Internal;
 
-internal sealed class ImporterInstance(Sender sender, ILogger<ImporterInstance> logger) : IDisposable
+internal sealed class ImporterInstance(ImporterArgs args, Sender sender, ILogger<ImporterInstance> logger) : IDisposable
 {
-  private readonly ILogger _logger = logger;
   private readonly RhinoCore _rhinoInstance = new(["/netcore-8"], WindowStyle.NoWindow);
 
-  private RhinoDoc? _rhinoDoc;
+  private readonly RhinoDoc _rhinoDoc = OpenDocument(args, logger);
 
-  public async Task<ImporterResponse> Run(ImporterArgs args, CancellationToken cancellationToken)
-  {
-    using var scopeJobId = ActivityScope.SetTag("jobId", args.JobId);
-    // using var scopeJobType = ActivityScope.SetTag("jobType", a.JobType);
-    using var scopeAttempt = ActivityScope.SetTag("job.attempt", args.Attempt.ToString());
-    using var scopeServerUrl = ActivityScope.SetTag("serverUrl", args.Account.serverInfo.url);
-    using var scopeProjectId = ActivityScope.SetTag("projectId", args.Project.id);
-    using var scopeModelId = ActivityScope.SetTag("modelId", args.ModelId);
-    using var scopeBlobId = ActivityScope.SetTag("blobId", args.BlobId);
-    using var scopeFileType = ActivityScope.SetTag("fileType", Path.GetExtension(args.FilePath).TrimStart('.'));
-    UserActivityScope.AddUserScope(args.Account);
-
-    var result = await TryImport(args, cancellationToken);
-    return result;
-  }
+  private readonly IReadOnlyList<IDisposable> _scopes =
+  [
+    ActivityScope.SetTag("jobId", args.JobId),
+    ActivityScope.SetTag("job.attempt", args.Attempt.ToString()),
+    // ActivityScope.SetTag("jobType", args.JobType),
+    ActivityScope.SetTag("serverUrl", args.Account.serverInfo.url),
+    ActivityScope.SetTag("projectId", args.Project.id),
+    ActivityScope.SetTag("modelId", args.ModelId),
+    ActivityScope.SetTag("blobId", args.BlobId),
+    ActivityScope.SetTag("fileType", Path.GetExtension(args.FilePath).TrimStart('.')),
+    UserActivityScope.AddUserScope(args.Account),
+  ];
 
-  [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "IPC")]
-  private async Task<ImporterResponse> TryImport(ImporterArgs args, CancellationToken cancellationToken)
+  public async Task<Version> RunRhinoImport(CancellationToken cancellationToken)
   {
     try
     {
-      var version = await RunRhinoImport(args, cancellationToken);
-      return new ImporterResponse { Version = version, ErrorMessage = null };
-    }
-    catch (Exception ex)
-    {
-      _logger.LogWarning(ex, "Import attempt failed with exception");
-      return new ImporterResponse { ErrorMessage = ex.Message, Version = null };
-    }
-  }
-
-  private async Task<Version> RunRhinoImport(ImporterArgs args, CancellationToken cancellationToken)
-  {
-    try
-    {
-      using var config = GetConfig(Path.GetExtension(args.FilePath));
-      logger.LogInformation("Opening file {FilePath}", args.FilePath);
-
-      _rhinoDoc = config.OpenInHeadlessDocument(args.FilePath);
       RhinoDoc.ActiveDoc = _rhinoDoc;
 
-      var version = await sender.Send(args.Project, args.ModelId, args.Account, cancellationToken);
+      var version = await sender
+        .Send(args.Project, args.ModelId, args.Account, cancellationToken)
+        .ConfigureAwait(false);
       return version;
     }
     finally
@@ -66,6 +45,14 @@ private async Task<Version> RunRhinoImport(ImporterArgs args, CancellationToken
     }
   }
 
+  private static RhinoDoc OpenDocument(ImporterArgs args, ILogger logger)
+  {
+    using var config = GetConfig(Path.GetExtension(args.FilePath));
+    logger.LogInformation("Opening file {FilePath}", args.FilePath);
+    return config.OpenInHeadlessDocument(args.FilePath);
+  }
+
+  [Pure]
   private static IFileTypeConfig GetConfig(string extension) =>
     extension.ToLowerInvariant() switch
     {
@@ -83,5 +70,9 @@ public void Dispose()
     // https://discourse.mcneel.com/t/rhino-inside-fatal-app-crashes-when-disposing-headless-documents/208673
     _rhinoDoc?.Dispose();
     _rhinoInstance.Dispose();
+    foreach (var scope in _scopes)
+    {
+      scope.Dispose();
+    }
   }
 }

Importers/Rhino/Speckle.Importers.Rhino/Internal/ImporterInstanceFactory.cs

@@ -0,0 +1,8 @@
+using Microsoft.Extensions.Logging;
+
+namespace Speckle.Importers.Rhino.Internal;
+
+internal sealed class ImporterInstanceFactory(Sender sender, ILogger<ImporterInstance> logger)
+{
+  public ImporterInstance Create(ImporterArgs args) => new(args, sender, logger);
+}

Importers/Rhino/Speckle.Importers.Rhino/Internal/ServiceRegistration.cs

@@ -20,6 +20,7 @@ public static IServiceCollection AddRhinoImporter(this IServiceCollection servic
     services.AddTransient<Progress>();
     services.AddTransient<Sender>();
     services.AddTransient<ImporterInstance>();
+    services.AddTransient<ImporterInstanceFactory>();
 
     // override default thread context
     services.AddSingleton<IThreadContext>(new ImporterThreadContext());

Importers/Rhino/Speckle.Importers.Rhino/Program.cs

@@ -1,9 +1,11 @@
-using System.Text.Json;
+using System.Diagnostics.CodeAnalysis;
+using System.Text.Json;
 using System.Text.Json.Serialization;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using RhinoInside;
 using Speckle.Importers.Rhino.Internal;
+using Version = Speckle.Sdk.Api.GraphQL.Models.Version;
 
 namespace Speckle.Importers.Rhino;
 
@@ -18,9 +20,12 @@ static Program()
     Console.WriteLine($"Loading Rhino @ {Resolver.RhinoSystemDirectory}");
   }
 
+  [SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "IPC")]
   public static async Task Main(string[] args)
   {
     ILogger? logger = null;
+    ImporterInstance? importer = null;
+
     try
     {
       var importerArgs = JsonSerializer.Deserialize<ImporterArgs>(args[0], s_serializerOptions);
@@ -32,13 +37,34 @@ public static async Task Main(string[] args)
       TaskScheduler.UnobservedTaskException += (_, eventArgs) =>
         logger.LogCritical(eventArgs.Exception, "Unobserved Task Exception");
 
-      var importer = serviceProvider.GetRequiredService<ImporterInstance>();
+      var factory = serviceProvider.GetRequiredService<ImporterInstanceFactory>();
+
+      // Error handling flow below here looks a bit of a mess, but we're having to navigate threading issues with rhino inside
+      // https://discourse.mcneel.com/t/rhino-inside-fatal-app-crashes-when-disposing-headless-documents/208673/7
+      try
+      {
+        // This needs to be called on the main thread
+        importer = factory.Create(importerArgs);
+      }
+      catch (Exception ex)
+      {
+        WriteResult(new() { ErrorMessage = ex.Message }, importerArgs.ResultsPath);
+        return;
+      }
 
+      // As soon as the main thread is yielded, it will be hogged by Rhino
+      // Task.Run ensures we run everything on a thread pool thread.
       await Task.Run(async () =>
         {
-          var result = await importer.Run(importerArgs, CancellationToken.None);
-          var serializedResult = JsonSerializer.Serialize(result, s_serializerOptions);
-          File.WriteAllLines(importerArgs.ResultsPath, [serializedResult]);
+          try
+          {
+            Version result = await importer.RunRhinoImport(CancellationToken.None).ConfigureAwait(false);
+            WriteResult(new() { Version = result }, importerArgs.ResultsPath);
+          }
+          catch (Exception ex)
+          {
+            WriteResult(new() { ErrorMessage = ex.Message }, importerArgs.ResultsPath);
+          }
         })
         .ConfigureAwait(false);
     }
@@ -53,7 +79,18 @@ await Task.Run(async () =>
       {
         Console.WriteLine(MESSAGE);
       }
+
       throw;
     }
+    finally
+    {
+      importer?.Dispose();
+    }
+  }
+
+  private static void WriteResult(ImporterResponse result, string resultsPath)
+  {
+    var serializedResult = JsonSerializer.Serialize(result, s_serializerOptions);
+    File.WriteAllLines(resultsPath, [serializedResult]);
   }
 }