From a50809bc91506fe75cf23629e2c245eb06c60d16 Mon Sep 17 00:00:00 2001 From: Jedd Morgan <45512892+JR-Morgan@users.noreply.github.com> Date: Mon, 16 Jun 2025 13:00:44 +0100 Subject: [PATCH] refactor!: Major code cleanup for clearer state (#27) * Quick pass * no locks * github actions * publish via github actions * pack * Parent non-optional * Generics are fun * second pass * Group workers, CTS, and Task in a class for simpler nullability * pollysharp * reverted cancellation changes * reset state * Update samples * changes * still working * the fix * private workers * comments * comments 2 * comments 3 * public worker count --- .../GH_AsyncComponent.cs | 188 ++++++++++-------- .../GrasshopperAsyncComponent.csproj | 1 + GrasshopperAsyncComponent/WorkerInstance.cs | 24 ++- .../Sample_PrimeCalculatorAsyncComponent.cs | 54 ++--- .../Sample_UslessCyclesComponent.cs | 44 ++-- 5 files changed, 178 insertions(+), 133 deletions(-) mode change 100755 => 100644 GrasshopperAsyncComponentDemo/SampleImplementations/Sample_UslessCyclesComponent.cs diff --git a/GrasshopperAsyncComponent/GH_AsyncComponent.cs b/GrasshopperAsyncComponent/GH_AsyncComponent.cs index 60a6681..be6c289 100755 --- a/GrasshopperAsyncComponent/GH_AsyncComponent.cs +++ b/GrasshopperAsyncComponent/GH_AsyncComponent.cs @@ -1,51 +1,76 @@ -using System.Collections.Concurrent; +using System.Collections.Concurrent; using System.Diagnostics; using Grasshopper.Kernel; using Timer = System.Timers.Timer; namespace GrasshopperAsyncComponent; +internal sealed class Worker : IDisposable + where T : GH_Component +{ + public required WorkerInstance Instance { get; init; } + + public required Task Task { get; init; } + + public required CancellationTokenSource CancellationSource { get; init; } + + public void Cancel() => CancellationSource.Cancel(); + + public void Dispose() + { + if (Task.IsCompleted) + { + Task.Dispose(); + } + CancellationSource.Dispose(); + } +} + /// /// Inherit your component from this class to make all the async goodness available. /// -public abstract class GH_AsyncComponent : GH_Component, IDisposable +public abstract class GH_AsyncComponent : GH_Component, IDisposable + where T : GH_Component { //List<(string, GH_RuntimeMessageLevel)> Errors; private readonly Action _reportProgress; - public ConcurrentDictionary ProgressReports { get; protected set; } - - private readonly Action _done; + public ConcurrentDictionary ProgressReports { get; } private readonly Timer _displayProgressTimer; - private int _state; + /// + /// a counter, used to count up the number of workers that have completed, + /// until _setData is set true, when it starts to count down the workers as their data is set. + /// + private volatile int _state; - private int _setData; + /// + /// functionally, a boolean, 1 or 0; + /// it will be set to 1 once all workers are ready for SetData to be called on them, then set back to 0. + /// + private volatile int _setData; - public List Workers { get; protected set; } + private readonly List> _workers; - private readonly List _tasks; - - public List CancellationSources { get; } + public int WorkerCount => _workers.Count; /// /// Set this property inside the constructor of your derived component. /// - public WorkerInstance? BaseWorker { get; set; } + public WorkerInstance? BaseWorker { get; set; } /// /// Optional: if you have opinions on how the default system task scheduler should treat your workers, set it here. /// - public TaskCreationOptions? TaskCreationOptions { get; set; } + public TaskCreationOptions TaskCreationOptions { get; set; } = TaskCreationOptions.None; protected GH_AsyncComponent(string name, string nickname, string description, string category, string subCategory) : base(name, nickname, description, category, subCategory) { - Workers = new List(); - CancellationSources = new List(); - _tasks = new List(); + _workers = new List>(); + ProgressReports = new ConcurrentDictionary(); _displayProgressTimer = new Timer(333) { AutoReset = false }; @@ -59,36 +84,36 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable _displayProgressTimer.Start(); } }; + } - _done = () => + private void Done() + { + Interlocked.Increment(ref _state); + if (_state == _workers.Count && _setData == 0) { - Interlocked.Increment(ref _state); - if (_state == Workers.Count && _setData == 0) - { - Interlocked.Exchange(ref _setData, 1); + Interlocked.Exchange(ref _setData, 1); - // We need to reverse the workers list to set the outputs in the same order as the inputs. - Workers.Reverse(); + // We need to reverse the workers list to set the outputs in the same order as the inputs. + _workers.Reverse(); - Rhino.RhinoApp.InvokeOnUiThread( - (Action) - delegate - { - ExpireSolution(true); - } - ); - } - }; + Rhino.RhinoApp.InvokeOnUiThread( + (Action) + delegate + { + ExpireSolution(true); + } + ); + } } public virtual void DisplayProgress(object sender, System.Timers.ElapsedEventArgs e) { - if (Workers.Count == 0 || ProgressReports.Values.Count == 0) + if (_workers.Count == 0 || ProgressReports.Values.Count == 0) { return; } - if (Workers.Count == 1) + if (_workers.Count == 1) { Message = ProgressReports.Values.Last().ToString("0.00%"); } @@ -100,7 +125,7 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable total += kvp.Value; } - Message = (total / Workers.Count).ToString("0.00%"); + Message = (total / _workers.Count).ToString("0.00%"); } Rhino.RhinoApp.InvokeOnUiThread( @@ -121,29 +146,24 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable Debug.WriteLine("Killing"); - foreach (var source in CancellationSources) + foreach (var currentWorker in _workers) { - source.Cancel(); + currentWorker.Cancel(); } - CancellationSources.Clear(); - Workers.Clear(); - ProgressReports.Clear(); - _tasks.Clear(); - - Interlocked.Exchange(ref _state, 0); + ResetState(); } protected override void AfterSolveInstance() { - System.Diagnostics.Debug.WriteLine("After solve instance was called " + _state + " ? " + Workers.Count); + Debug.WriteLine("After solve instance was called " + _state + " ? " + _workers.Count); // We need to start all the tasks as close as possible to each other. - if (_state == 0 && _tasks.Count > 0 && _setData == 0) + if (_state == 0 && _workers.Count > 0 && _setData == 0) { - System.Diagnostics.Debug.WriteLine("After solve INVOKATION"); - foreach (var task in _tasks) + Debug.WriteLine("After solve INVOCATION"); + foreach (var worker in _workers) { - task.Start(); + worker.Task.Start(); } } } @@ -170,31 +190,30 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable // Add cancellation source to our bag var tokenSource = new CancellationTokenSource(); - CancellationSources.Add(tokenSource); var currentWorker = BaseWorker.Duplicate($"Worker-{da.Iteration}", tokenSource.Token); - if (currentWorker == null) - { - AddRuntimeMessage(GH_RuntimeMessageLevel.Error, "Could not get a worker instance."); - return; - } // Let the worker collect data. currentWorker.GetData(da, Params); - var currentRun = - TaskCreationOptions != null - ? new Task( - () => currentWorker.DoWork(_reportProgress, _done), - tokenSource.Token, - (TaskCreationOptions)TaskCreationOptions - ) - : new Task(() => currentWorker.DoWork(_reportProgress, _done), tokenSource.Token); + var currentRun = new Task( + async () => + { + await currentWorker.DoWork(_reportProgress, Done).ConfigureAwait(true); + }, + tokenSource.Token, + TaskCreationOptions + ); // Add the worker to our list - Workers.Add(currentWorker); - - _tasks.Add(currentRun); + _workers.Add( + new() + { + Instance = currentWorker, + Task = currentRun, + CancellationSource = tokenSource, + } + ); return; } @@ -204,10 +223,10 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable return; } - if (Workers.Count > 0) + if (_workers.Count > 0) { Interlocked.Decrement(ref _state); - Workers[_state].SetData(da); + _workers[_state].Instance.SetData(da); } if (_state != 0) @@ -215,31 +234,34 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable return; } - CancellationSources.Clear(); - Workers.Clear(); - ProgressReports.Clear(); - _tasks.Clear(); + foreach (var worker in _workers) + { + worker?.Dispose(); + } - Interlocked.Exchange(ref _setData, 0); + ResetState(); Message = "Done"; OnDisplayExpired(true); } - public void RequestCancellation() + private void ResetState() { - foreach (var source in CancellationSources) - { - source.Cancel(); - } - - CancellationSources.Clear(); - Workers.Clear(); + _workers.Clear(); ProgressReports.Clear(); - _tasks.Clear(); Interlocked.Exchange(ref _state, 0); Interlocked.Exchange(ref _setData, 0); + } + + public void RequestCancellation() + { + foreach (var worker in _workers) + { + worker.Cancel(); + } + + ResetState(); Message = "Cancelled"; OnDisplayExpired(true); } @@ -260,6 +282,10 @@ public abstract class GH_AsyncComponent : GH_Component, IDisposable if (disposing) { + foreach (var worker in _workers) + { + worker?.Dispose(); + } _displayProgressTimer?.Dispose(); } } diff --git a/GrasshopperAsyncComponent/GrasshopperAsyncComponent.csproj b/GrasshopperAsyncComponent/GrasshopperAsyncComponent.csproj index 21e9a93..35d236e 100755 --- a/GrasshopperAsyncComponent/GrasshopperAsyncComponent.csproj +++ b/GrasshopperAsyncComponent/GrasshopperAsyncComponent.csproj @@ -21,6 +21,7 @@ + diff --git a/GrasshopperAsyncComponent/WorkerInstance.cs b/GrasshopperAsyncComponent/WorkerInstance.cs index cd21e0c..66396ff 100755 --- a/GrasshopperAsyncComponent/WorkerInstance.cs +++ b/GrasshopperAsyncComponent/WorkerInstance.cs @@ -1,16 +1,17 @@ -using Grasshopper.Kernel; +using Grasshopper.Kernel; namespace GrasshopperAsyncComponent; /// -/// A class that holds the actual compute logic and encapsulates the state it needs. Every needs to have one. +/// A class that holds the actual compute logic and encapsulates the state it needs. Every needs to have one. /// -public abstract class WorkerInstance(GH_Component? parent, string id, CancellationToken cancellationToken) +public abstract class WorkerInstance(T parent, string id, CancellationToken cancellationToken) + where T : GH_Component { /// /// The parent component. Useful for passing state back to the host component. /// - public GH_Component? Parent { get; set; } = parent; + public T Parent { get; set; } = parent; public CancellationToken CancellationToken { get; } = cancellationToken; @@ -22,24 +23,29 @@ public abstract class WorkerInstance(GH_Component? parent, string id, Cancellati /// A Unique id for the new duplicate /// A cancellationToken to be passed to the new duplicate /// - public abstract WorkerInstance? Duplicate(string id, CancellationToken cancellationToken); + public abstract WorkerInstance Duplicate(string id, CancellationToken cancellationToken); /// /// This method is where the actual calculation/computation/heavy lifting should be done. - /// Make sure you always check as frequently as you can if is cancelled. For an example, see the PrimeCalculatorWorker example. + /// Make sure you always check as frequently as you can if is cancelled. For an example, see the PrimeCalculatorWorker example. /// + /// + /// If you don't need function, then you can simply return + /// Either way, you should be sure to handle exceptions in this function. Otherwise, they will be Unobserved! + /// You can call on s, but avoid calling it when cancellation is has been observed. + /// /// Call this to report progress up to the parent component. /// Call this when everything is done. It will tell the parent component that you're ready to . - public abstract void DoWork(Action reportProgress, Action done); + public abstract Task DoWork(Action reportProgress, Action done); /// - /// Write your data setting logic here. Do not call this function directly from this class. It will be invoked by the parent after you've called `Done` in the function. + /// Write your data setting logic here. Do not call this function directly from this class. It will be invoked by the parent after you've called `Done` in the function. /// /// public abstract void SetData(IGH_DataAccess da); /// - /// Write your data collection logic here. Do not call this method directly. It will be invoked by the parent . + /// Write your data collection logic here. Do not call this method directly. It will be invoked by the parent . /// /// /// diff --git a/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_PrimeCalculatorAsyncComponent.cs b/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_PrimeCalculatorAsyncComponent.cs index e1bacb0..1895e19 100755 --- a/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_PrimeCalculatorAsyncComponent.cs +++ b/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_PrimeCalculatorAsyncComponent.cs @@ -1,10 +1,10 @@ -using System.Windows.Forms; +using System.Windows.Forms; using Grasshopper.Kernel; using GrasshopperAsyncComponent; namespace GrasshopperAsyncComponentDemo.SampleImplementations; -public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent +public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent { public override Guid ComponentGuid => new Guid("22C612B0-2C57-47CE-B9FE-E10621F18933"); @@ -46,25 +46,38 @@ public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent ); } - private sealed class PrimeCalculatorWorker : WorkerInstance + private sealed class PrimeCalculatorWorker : WorkerInstance { private int TheNthPrime { get; set; } = 100; private long ThePrime { get; set; } = -1; public PrimeCalculatorWorker( - GH_Component? parent, + Sample_PrimeCalculatorAsyncComponent parent, string id = "baseworker", CancellationToken cancellationToken = default ) : base(parent, id, cancellationToken) { } - public override void DoWork(Action reportProgress, Action done) + public override Task DoWork(Action reportProgress, Action done) + { + try + { + CalculatePrimes(reportProgress); + done(); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + //No need to call `done()` - GrasshopperAsyncComponent assumes immediate cancel, + //thus it has already performed clean-up actions that would normally be done on `done()` + } + + return Task.CompletedTask; + } + + public void CalculatePrimes(Action reportProgress) { // 👉 Checking for cancellation! - if (CancellationToken.IsCancellationRequested) - { - return; - } + CancellationToken.ThrowIfCancellationRequested(); int count = 0; long a = 2; @@ -73,20 +86,14 @@ public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent while (count < TheNthPrime) { // 👉 Checking for cancellation! - if (CancellationToken.IsCancellationRequested) - { - return; - } + CancellationToken.ThrowIfCancellationRequested(); long b = 2; int prime = 1; // to check if found a prime while (b * b <= a) { // 👉 Checking for cancellation! - if (CancellationToken.IsCancellationRequested) - { - return; - } + CancellationToken.ThrowIfCancellationRequested(); if (a % b == 0) { @@ -106,11 +113,12 @@ public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent } ThePrime = --a; - done(); } - public override WorkerInstance Duplicate(string id, CancellationToken cancellationToken) => - new PrimeCalculatorWorker(Parent, id, cancellationToken); + public override WorkerInstance Duplicate( + string id, + CancellationToken cancellationToken + ) => new PrimeCalculatorWorker(Parent, id, cancellationToken); public override void GetData(IGH_DataAccess da, GH_ComponentParamServer parameters) { @@ -131,12 +139,6 @@ public class Sample_PrimeCalculatorAsyncComponent : GH_AsyncComponent public override void SetData(IGH_DataAccess da) { - // 👉 Checking for cancellation! - if (CancellationToken.IsCancellationRequested) - { - return; - } - da.SetData(0, ThePrime); } } diff --git a/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_UslessCyclesComponent.cs b/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_UslessCyclesComponent.cs old mode 100755 new mode 100644 index 6fc0466..0f1a87c --- a/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_UslessCyclesComponent.cs +++ b/GrasshopperAsyncComponentDemo/SampleImplementations/Sample_UslessCyclesComponent.cs @@ -1,10 +1,10 @@ -using System.Windows.Forms; +using System.Windows.Forms; using Grasshopper.Kernel; using GrasshopperAsyncComponent; namespace GrasshopperAsyncComponentDemo.SampleImplementations; -public class Sample_UselessCyclesAsyncComponent : GH_AsyncComponent +public class Sample_UselessCyclesAsyncComponent : GH_AsyncComponent { public override Guid ComponentGuid => new Guid("DF2B93E2-052D-4BE4-BC62-90DC1F169BF6"); @@ -19,20 +19,33 @@ public class Sample_UselessCyclesAsyncComponent : GH_AsyncComponent } private sealed class UselessCyclesWorker( - GH_Component? parent, + Sample_UselessCyclesAsyncComponent parent, string id = "baseworker", CancellationToken cancellationToken = default - ) : WorkerInstance(parent, id, cancellationToken) + ) : WorkerInstance(parent, id, cancellationToken) { private int MaxIterations { get; set; } = 100; - public override void DoWork(Action reportProgress, Action done) + public override Task DoWork(Action reportProgress, Action done) + { + try + { + RunUselessCycles(reportProgress); + done(); + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + //No need to call `done()` - GrasshopperAsyncComponent assumes immediate cancel, + //thus it has already performed clean-up actions that would normally be done on `done()` + } + + return Task.CompletedTask; + } + + public void RunUselessCycles(Action reportProgress) { // Checking for cancellation - if (CancellationToken.IsCancellationRequested) - { - return; - } + CancellationToken.ThrowIfCancellationRequested(); for (int i = 0; i <= MaxIterations; i++) { @@ -45,17 +58,14 @@ public class Sample_UselessCyclesAsyncComponent : GH_AsyncComponent reportProgress(Id, (i + 1) / (double)MaxIterations); // Checking for cancellation - if (CancellationToken.IsCancellationRequested) - { - return; - } + CancellationToken.ThrowIfCancellationRequested(); } - - done(); } - public override WorkerInstance Duplicate(string id, CancellationToken cancellationToken) => - new UselessCyclesWorker(Parent, id, cancellationToken); + public override WorkerInstance Duplicate( + string id, + CancellationToken cancellationToken + ) => new UselessCyclesWorker(Parent, id, cancellationToken); public override void GetData(IGH_DataAccess da, GH_ComponentParamServer parameters) {