From 78e8552ea4a454ecf793d7133220ceba52e7aa86 Mon Sep 17 00:00:00 2001 From: Eric Wolf Date: Fri, 7 Jul 2023 16:27:14 +0200 Subject: [PATCH] dispose wait set after invoking `IContext.OnShutdown` This prevents violating the documentation. --- src/ros2cs/ros2cs_core/Context.cs | 51 ++++++++++++++++++- src/ros2cs/ros2cs_core/WaitSet.cs | 42 ++++++++++++--- .../ros2cs_core/executors/ManualExecutor.cs | 2 +- src/ros2cs/ros2cs_tests/src/WaitSetTest.cs | 10 +++- 4 files changed, 93 insertions(+), 12 deletions(-) diff --git a/src/ros2cs/ros2cs_core/Context.cs b/src/ros2cs/ros2cs_core/Context.cs index 44adf86..07ba9a4 100644 --- a/src/ros2cs/ros2cs_core/Context.cs +++ b/src/ros2cs/ros2cs_core/Context.cs @@ -60,6 +60,14 @@ public sealed class Context : IContext /// private HashSet GuardConditions = new HashSet(); + /// + /// Collection of wait sets active in this context; + /// + /// + /// Also used for synchronisation when creating / removing guard conditions. + /// + private HashSet WaitSets = new HashSet(); + /// /// Get the current RMW implementation. /// @@ -171,6 +179,40 @@ internal bool RemoveGuardCondition(GuardCondition guardCondition) } } + /// + /// Create a wait set. + /// + /// + /// This method is thread safe. + /// + /// A new wait set instance. + internal WaitSet CreateWaitSet() + { + lock (this.WaitSets) + { + WaitSet waitSet = new WaitSet(this); + this.WaitSets.Add(waitSet); + return waitSet; + } + } + + /// + /// Remove a wait set. + /// + /// + /// This method is intended to be used by and does not dispose the wait set. + /// Furthermore, it is thread safe. + /// + /// Wait set to remove. + /// If the wait set existed in this context and has been removed. + internal bool RemoveWaitSet(WaitSet waitSet) + { + lock (this.WaitSets) + { + return this.WaitSets.Remove(waitSet); + } + } + /// /// This method is not thread safe. /// Do not call while the context or any entities @@ -197,7 +239,7 @@ private void Dispose(bool disposing) { Utils.CheckReturnEnum(ret); } - // only continue if ROSNodes and GuardConditions has not been finalized + // only continue if the collections of the active primitives have not been finalized if (disposing) { this.OnShutdown?.Invoke(); @@ -211,7 +253,12 @@ private void Dispose(bool disposing) guardCondition.DisposeFromContext(); } this.GuardConditions.Clear(); - // only safe when all nodes are gone, not calling Dispose() will leak the Handle + foreach (var waitSet in this.WaitSets) + { + waitSet.DisposeFromContext(); + } + this.WaitSets.Clear(); + // only safe when all primitives are gone, not calling Dispose() will leak the Handle Utils.CheckReturnEnum(NativeRcl.rcl_context_fini(this.Handle)); this.FreeHandles(); } diff --git a/src/ros2cs/ros2cs_core/WaitSet.cs b/src/ros2cs/ros2cs_core/WaitSet.cs index 9c9eb44..3bfa9c4 100644 --- a/src/ros2cs/ros2cs_core/WaitSet.cs +++ b/src/ros2cs/ros2cs_core/WaitSet.cs @@ -16,6 +16,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; namespace ROS2 @@ -49,7 +50,12 @@ internal sealed class WaitSet : IReadOnlyCollection, IExtendedDisposa /// /// Context associated with this wait set. /// - public IContext Context { get; private set; } + public IContext Context + { + get => this.ROSContext; + } + + private Context ROSContext; /// public bool IsDisposed @@ -99,7 +105,7 @@ public int Count /// Associated context internal WaitSet(Context context) { - this.Context = context; + this.ROSContext = context; this.Handle = NativeRclInterface.rclcs_get_zero_initialized_wait_set(); int ret = NativeRcl.rcl_wait_set_init( this.Handle, @@ -117,7 +123,6 @@ internal WaitSet(Context context) this.FreeHandles(); Utils.CheckReturnEnum(ret); } - context.OnShutdown += this.Dispose; } /// @@ -314,16 +319,37 @@ private void Dispose(bool disposing) return; } + if (disposing) + { + bool success = this.ROSContext.RemoveWaitSet(this); + Debug.Assert(success, "failed to remove wait set"); + this.ClearCollections(); + } + Utils.CheckReturnEnum(NativeRcl.rcl_wait_set_fini(this.Handle)); this.FreeHandles(); + } - if (disposing) + /// Dispose without modifying the context. + internal void DisposeFromContext() + { + if (this.Handle == IntPtr.Zero) { - this.Context.OnShutdown -= this.Dispose; - this.Subscriptions.Clear(); - this.Clients.Clear(); - this.Services.Clear(); + return; } + + this.ClearCollections(); + + Utils.CheckReturnEnum(NativeRcl.rcl_wait_set_fini(this.Handle)); + this.FreeHandles(); + } + + private void ClearCollections() + { + this.Subscriptions.Clear(); + this.Clients.Clear(); + this.Services.Clear(); + this.GuardConditions.Clear(); } private void FreeHandles() diff --git a/src/ros2cs/ros2cs_core/executors/ManualExecutor.cs b/src/ros2cs/ros2cs_core/executors/ManualExecutor.cs index d64b43a..3fc42d3 100644 --- a/src/ros2cs/ros2cs_core/executors/ManualExecutor.cs +++ b/src/ros2cs/ros2cs_core/executors/ManualExecutor.cs @@ -114,7 +114,7 @@ public bool IsReadOnly /// Context to associate with. /// If is disposed. public ManualExecutor(Context context) : this( - new WaitSet(context), + context.CreateWaitSet(), context.CreateGuardCondition(() => { }) ) { } diff --git a/src/ros2cs/ros2cs_tests/src/WaitSetTest.cs b/src/ros2cs/ros2cs_tests/src/WaitSetTest.cs index 2c023ae..a6fb0b2 100644 --- a/src/ros2cs/ros2cs_tests/src/WaitSetTest.cs +++ b/src/ros2cs/ros2cs_tests/src/WaitSetTest.cs @@ -36,7 +36,7 @@ public class WaitSetTest public void SetUp() { this.Context = new Context(); - this.WaitSet = new WaitSet(this.Context); + this.WaitSet = this.Context.CreateWaitSet(); } [TearDown] @@ -65,6 +65,14 @@ public void DoubleDisposeWaitSet() Assert.That(this.WaitSet.IsDisposed, Is.True); } + [Test] + public void OnShutdownDisposal() + { + this.Context.OnShutdown += () => Assert.That(this.WaitSet.IsDisposed, Is.False); + + this.Context.Dispose(); + } + [Test] public void TestSubscriptionCollection() {