regression of thread-safety for logging macros#393
Merged
Conversation
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Member
Author
|
CI:
|
clalancette
previously requested changes
Oct 14, 2022
Contributor
clalancette
left a comment
There was a problem hiding this comment.
In short, I'm fine with disabling this optimization for now.
However, I do think we should add additional language into the doc block for rcutils_log saying that it is expected to be thread-safe with itself. That might have saved me from introducing this problem to begin with (though the new test may also do that in the future).
With that change made, 👍 from me.
jacobperron
reviewed
Oct 14, 2022
fujitatomoya
approved these changes
Oct 14, 2022
Signed-off-by: William Woodall <william@osrfoundation.org>
jacobperron
approved these changes
Oct 14, 2022
Member
Author
|
I'm not going to re-run ci.ros2.org because the changes were docs only and the linters are run in the Rpr job. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This came up in one of our projects using rolling and they noticed an uptick in crashes originating in logging calls.
It turns out it is related to optimizations made in #381, and is only triggered in specific scenarios, namely when some loggers have had their severity set by the user, but not all the ones being called. This triggers a code path that was part of #381 which can cause a global hash map to be added to, and sometimes grown, and this seems to be the root of many of the crashes, though it crashes in many different ways.
In this pr I added a regression test that catches these kind of problems most runs, but it isn't perfect since it is a race condition.
After some investigation and talking with @clalancette, this code block seems to be the offender:
rcutils/src/logging.c
Lines 946 to 957 in 3485f1b
After removing that, this test passes even if I use
--gtest_repeat=1000to run it 1000 times. Whereas it fails with that code uncommented almost every time. So I don't know if this makes the logging macros completely (conceptually) thread-safe, but it does undo this regression, it seems.