Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve lifecycle #38

Open
Deric-W opened this issue Dec 19, 2022 · 2 comments
Open

Improve lifecycle #38

Deric-W opened this issue Dec 19, 2022 · 2 comments

Comments

@Deric-W
Copy link
Contributor

Deric-W commented Dec 19, 2022

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
Copy link
Member

@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
Copy link
Contributor Author

Deric-W commented Jan 27, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants