RobotecAI / ros2cs

A C# (.Net) implementation of ros2 client library (rcl), enabling communication between ros2 ecosystem and C#/.Net applications such as Unity3D
Apache License 2.0
89 stars 22 forks source link

Improve lifecycle #38

Open Deric-W opened 1 year ago

Deric-W commented 1 year ago

Hello, I noticed that nodes, publishers, services and clients expose a void Dispose() method which is used for disposing resources while also having methods like bool RemovePublisher(IPublisherBase) which should be called on other objects to remove them and as implemented in #37 call Dispose().

While users should call this remove methods it is possible that they just call Dispose() (for example when using using ) which can lead to disposed objects being kept around until the object containing their remove method is disposed.

Furthermore, most disposable objects call Dispose() in their finalizers to account for users forgetting to call it. Since accessing other managed objects in finalizers is tricky it should not be done, which means that for example accessing the collection of publishers associated with a Node during finalization is risky (the collection and its managed internals having no finalizers is an implementation detail and should not be relied on as far as I know).

Possible Solution

The remove methods should be removed and the Dispose() method of the objects to be removed should handle all cleanup, which allows users to use using and prevents a node for example from being collected by the GC while a publisher exists.

Furthermore, the Ros2cs class should be replaced by an non-static class to prevent confusion which code is responsible for calling Ros2cs.Shutdown() and allows implementing IExtendedDisposable.

The problem of accessing managed objects during finalization can be solved by allowing some resources to leak if Dispose() was not called, which may be acceptable since node and context finalization typically happen on application shutdown. Resources leaked include context and node handles, since we can not assure that associated resources are disposed (the context can however be shut down). Handles of resources like publishers can be finalized during finalization.

Unity integration can be accomplished by calling Dispose() in ROS2UnityComponent but finalizing ROS2UnityCore and ROS2Node by GC would be difficult, which could be solved by removing them.

This is a class diagram containing the new methods used for lifecycle management (Publisher as an example of the other resources): Lifecycle drawio

I could implement these changes if you agree with my ideas.

adamdbrw commented 1 year ago

@Deric-W these seem to be beneficial changes. So far we were only interested in disposing of subscribers and publishers during runtime, not the nodes themselves. Could you submit a PR?

Deric-W commented 1 year ago

Iam currently working on it here and can make a PR when I reach an acceptable state.