-
Notifications
You must be signed in to change notification settings - Fork 22
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
Comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hello,
I noticed that nodes, publishers, services and clients expose a
void Dispose()
method which is used for disposing resources while also having methods likebool RemovePublisher(IPublisherBase)
which should be called on other objects to remove them and as implemented in #37 callDispose()
.While users should call this remove methods it is possible that they just call
Dispose()
(for example when usingusing
) 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 useusing
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 callingRos2cs.Shutdown()
and allows implementingIExtendedDisposable
.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):I could implement these changes if you agree with my ideas.
The text was updated successfully, but these errors were encountered: