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

Use RTLD_NODELETE=true when loading libraries #1649

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Aug 16, 2022

Fixes (Potentially): #806

Summary

Gazebo Garden crashes at exit because the destructor for an EventT object created by a gui plugin is accesses after the plugin is unloaded. For that particular crash, the plugin in question is in gz-gui, so I've submitted a fix there (gazebosim/gz-gui#469). This PR is proactively using RTLD_NODELETE to prevent future bugs of this nature.

I believe this might also fix #806 because if the shared libraries never get deleted, when a plugin goes through a load--unload--load cycle, the components (more specifically, the vtable of the components) it registers will be located in the same memory address. This was not the case before, which I think was the culprit for many of the flakiness in macOS.

More context:

Requires:

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey requested a review from chapulina as a code owner August 16, 2022 20:59
@azeey azeey self-assigned this Aug 16, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 16, 2022
@azeey azeey added bug Something isn't working needs upstream release Blocked by a release of an upstream library and removed 🌱 garden Ignition Garden labels Aug 16, 2022
@chapulina chapulina added the 🌱 garden Ignition Garden label Aug 16, 2022
@nkoenig nkoenig self-requested a review August 22, 2022 18:28
@chapulina chapulina added Breaking change Breaks API, ABI or behavior. Must target unstable version. and removed needs upstream release Blocked by a release of an upstream library labels Aug 29, 2022
@chapulina
Copy link
Contributor

@osrf-jenkins run tests please

/// TODO (azeey) Temporarliy disabled until
/// https://github.com/gazebosim/gz-sim/issues/1443 is resolved
TEST_P(GazeboDeathTest, DISABLED_CleanExit)
TEST_P(GazeboDeathTest, CleanExit)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is failing on all platforms 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this is using nightlies of plugin2, but not of gui7 yet, let me trigger some:

  • Build Status
  • Build Status

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, now it passes on Ubuntu, but it's failing on macOS:

61: [ RUN      ] WorldFiles/GazeboDeathTest.CleanExit/0
61: 
61: [WARNING] /Users/jenkins/workspace/ignition_gazebo-ci-pr_any-homebrew-amd64/ign-gazebo/test/gtest_vendor/src/gtest-death-test.cc:1108:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 3 threads. See https://github.com/google/googletest/blob/master/docs/advanced.md#death-tests-and-threads for more explanation and suggested solutions, especially if this is the last message you see before your test times out.
61: /Users/jenkins/workspace/ignition_gazebo-ci-pr_any-homebrew-amd64/ign-gazebo/src/gui/Gui_clean_exit_TEST.cc:97: Failure
61: Death test: startBoth(sdfFile)
61:     Result: died but not with expected exit code:
61:             Terminated by signal 11
61: Actual msg:
61: [  DEATH   ] The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
61: [  DEATH   ] Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.
61: [  DEATH   ] 
61: [  FAILED  ] WorldFiles/GazeboDeathTest.CleanExit/0, where GetParam() = "empty.sdf" (50 ms)

Windows is also failing, but it doesn't even find the shared libraries.

I'm going to disable the test for macOS and Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also only enabled on Linux in Fortress, so thanks for making the change in f0c2c27

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #1649 (5118037) into gz-sim7 (0942c2e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 5118037 differs from pull request most recent head f0c2c27. Consider uploading reports for the commit f0c2c27 to get more accurate results

@@           Coverage Diff            @@
##           gz-sim7    #1649   +/-   ##
========================================
  Coverage    63.90%   63.90%           
========================================
  Files          334      334           
  Lines        26344    26344           
========================================
  Hits         16836    16836           
  Misses        9508     9508           
Impacted Files Coverage Δ
src/SystemLoader.cc 54.54% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina merged commit ff1c82b into gazebosim:gz-sim7 Sep 1, 2022
@azeey azeey deleted the plugin_nodelete branch September 2, 2022 17:36
@j-rivero j-rivero mentioned this pull request Sep 16, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change Breaks API, ABI or behavior. Must target unstable version. bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants