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

purge meta objects within the graveyard if the library is unassociated #200

Closed

Conversation

iuhilnehc-ynos
Copy link

It seems that #196 is not updated as suggested by the maintainer for a few months. I would like to add @shshlei a co-author to create this PR to fix ros2/rcl#1009.

If this is not the correct way, please feel free to close it.

Co-authored-by: shshlei [email protected]
Signed-off-by: Chen Lihui [email protected]

Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not expert for this repository, but it looks good to me to do so.

@fujitatomoya
Copy link

@gbiggs @mjcarroll either of you, could you take a look at this? this is to address memory leak.

@iuhilnehc-ynos iuhilnehc-ynos changed the title purge meta objects whthin the graveyard if the library is unassociated purge meta objects within the graveyard if the library is unassociated Sep 19, 2022
@@ -538,6 +538,7 @@ void unloadLibrary(const std::string & library_path, ClassLoader * loader)

library->unload_library();
itr = open_libraries.erase(itr);
purgeGraveyardOfMetaobjects(library_path, loader, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this definitely fixes the leak for me, I'll say that it I'm somewhat concerned about the impact of this. Just above, we are calling destroyMetaObjectsForLibrary, which may end up placing the meta-objects for this library into the graveyard. Then we are immediately purging them here. That begs the question: why put them into the graveyard at all?

Further, there is this comment in destroyMetaObjectsForLibrary:

        // Insert into graveyard
        // We remove the metaobject from its factory map, but we don't destroy it...instead it
        // saved to a "graveyard" to the side.
        // This is due to our static global variable initialization problem that causes factories
        // to not be registered when a library is closed and then reopened.
        // This is because it's truly not closed due to the use of global symbol binding i.e.
        // calling dlopen with RTLD_GLOBAL instead of RTLD_LOCAL.
        // We require using the former as the which is required to support RTTI

Which suggests that maybe we need to do additional testing about closing/reopening libraries with this? I'm honestly not sure, but I'm not too familiar with this repository. It would be great to have feedback from one of the maintainers @gbiggs or @mjcarroll .

@iuhilnehc-ynos
Copy link
Author

Close in favor of #201

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

Successfully merging this pull request may close these issues.

3 participants