Skip to content

Commit

Permalink
dispose guard conditions after invoking IContext.OnShutdown
Browse files Browse the repository at this point in the history
This prevents violating the documentation.
  • Loading branch information
Deric-W committed Jul 7, 2023
1 parent da31e2e commit d004d9e
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 10 deletions.
50 changes: 49 additions & 1 deletion src/ros2cs/ros2cs_core/Context.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ public sealed class Context : IContext
/// </remarks>
private Dictionary<string, Node> ROSNodes = new Dictionary<string, Node>();

/// <summary>
/// Collection of guard conditions active in this context.
/// </summary>
/// <remarks>
/// Also used for synchronisation when creating / removing guard conditions.
/// </remarks>
private HashSet<GuardCondition> GuardConditions = new HashSet<GuardCondition>();

/// <summary>
/// Get the current RMW implementation.
/// </summary>
Expand Down Expand Up @@ -128,6 +136,41 @@ internal bool RemoveNode(string name)
}
}

/// <summary>
/// Create a guard condition.
/// </summary>
/// <remarks>
/// This method is thread safe.
/// </remarks>
/// <param name="callback"> Callback executed by the executor when the guard condition is triggered. </param>
/// <returns> A new guard condition instance. </returns>
internal GuardCondition CreateGuardCondition(Action callback)
{
lock (this.GuardConditions)
{
GuardCondition guardCondition = new GuardCondition(this, callback);
this.GuardConditions.Add(guardCondition);
return guardCondition;
}
}

/// <summary>
/// Remove a guard condition.
/// </summary>
/// <remarks>
/// This method is intended to be used by <see cref="GuardCondition.Dispose"/> and does not dispose the guard condition.
/// Furthermore, it is thread safe.
/// </remarks>
/// <param name="guardCondition"> Guard condition to remove. </param>
/// <returns> If the guard condition existed in this context and has been removed. </returns>
internal bool RemoveGuardCondition(GuardCondition guardCondition)
{
lock (this.GuardConditions)
{
return this.GuardConditions.Remove(guardCondition);
}
}

/// <remarks>
/// This method is not thread safe.
/// Do not call while the context or any entities
Expand All @@ -154,7 +197,7 @@ private void Dispose(bool disposing)
{
Utils.CheckReturnEnum(ret);
}
// only continue if ROSNodes has not been finalized
// only continue if ROSNodes and GuardConditions has not been finalized
if (disposing)
{
this.OnShutdown?.Invoke();
Expand All @@ -163,6 +206,11 @@ private void Dispose(bool disposing)
node.DisposeFromContext();
}
this.ROSNodes.Clear();
foreach (var guardCondition in this.GuardConditions)
{
guardCondition.DisposeFromContext();
}
this.GuardConditions.Clear();
// only safe when all nodes are gone, not calling Dispose() will leak the Handle
Utils.CheckReturnEnum(NativeRcl.rcl_context_fini(this.Handle));
this.FreeHandles();
Expand Down
23 changes: 18 additions & 5 deletions src/ros2cs/ros2cs_core/GuardCondition.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

using System;
using System.Diagnostics;

namespace ROS2
{
Expand Down Expand Up @@ -68,7 +69,6 @@ out IntPtr handle
}
Utils.CheckReturnEnum(ret);
this.Handle = handle;
context.OnShutdown += this.Dispose;
}

/// <summary>
Expand Down Expand Up @@ -123,13 +123,26 @@ private void Dispose(bool disposing)
return;
}

Utils.CheckReturnEnum(NativeRcl.rcl_guard_condition_fini(this.Handle));
this.FreeHandles();

// only do if Context.GuardConditions has not been finalized
if (disposing)
{
this.Context.OnShutdown -= this.Dispose;
bool success = this.Context.RemoveGuardCondition(this);
Debug.Assert(success, message: "failed to remove guard condition");
}

this.DisposeFromContext();
}

/// <summary> Dispose without modifying the context. </summary>
internal void DisposeFromContext()
{
if (this.Handle == IntPtr.Zero)
{
return;
}

Utils.CheckReturnEnum(NativeRcl.rcl_guard_condition_fini(this.Handle));
this.FreeHandles();
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/ros2cs/ros2cs_core/executors/ManualExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public bool IsReadOnly
/// <exception cref="ObjectDisposedException"> If <paramref name="context"/> is disposed. </exception>
public ManualExecutor(Context context) : this(
new WaitSet(context),
new GuardCondition(context, () => { })
context.CreateGuardCondition(() => { })
)
{ }

Expand Down
5 changes: 2 additions & 3 deletions src/ros2cs/ros2cs_tests/src/GuardConditionTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ public class GuardConditionTest
public void SetUp()
{
this.Context = new Context();
this.GuardCondition = new GuardCondition(
this.Context,
this.GuardCondition = this.Context.CreateGuardCondition(
() => { throw new InvalidOperationException("guard condition was called"); }
);
}
Expand Down Expand Up @@ -66,7 +65,7 @@ public void DisposedContextHandling()
{
this.Context.Dispose();

Assert.Throws<ObjectDisposedException>(() => new GuardCondition(this.Context, () => {}));
Assert.Throws<ObjectDisposedException>(() => this.Context.CreateGuardCondition(() => { }));
}

[Test]
Expand Down

0 comments on commit d004d9e

Please sign in to comment.