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.
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
Fault injection macros and functionality (plus example) #264
Fault injection macros and functionality (plus example) #264
Changes from 2 commits
153f42e
e9cc019
97dc369
a4f74ae
adad3bb
e86483a
e083a0d
203747d
9a72ee0
f5bb491
36050ca
2e99f4c
75009d9
b03d89e
6a5daad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brawner I think this the a strong (the strongest?) constrain. This will cover 90% of our use cases, but, in the most general case, not all errors occur during early checks and not all errors can (should?) have zero observable side-effects. Could the macro take some statements to mimic such side effects if any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that may be a reasonable extension to add. The difficulty though is that there may be multiple instances of returning the same error code in a function (for example
RCUTILS_RET_ERROR
orRCUTILS_BAD_ALLOC
), then the side effects may be different depending on where it returns.While many cases might have side effects after returning through an error case, my limited experience with this code base suggests those typically result in an undefined state more than an expected partially completed state.
I think for this case, we'll just have to build up a list of examples and what the requirements for those functions would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I'd like to point out though that while:
often holds true, IMHO it's bad practice. Really bad practice. If your program is left in an undefined state after returning an error, you may as well terminate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta: that's not always true or possible, finalization functions being a good counterexample. I don't think the limitation should be addressed here though (or ever if mocks help us cover those cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brawner one thing this scheme cannot do is force an error during cleanup after one or more errors have been forced. What about if, in addition to forcing a single function to fail, we make all following functions fail as well? Like a flag to prevent the count to ever be decremented below 0.
I envision the
RCUTILS_CHECK_FAULT_SAFETY
macro below taking an optional FLUKE or MASSIVE argument for each case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some examples where a failure in one location is likely to result in a failure in another location. For example, allocations may continue to fail, or file system functions may continue to fail. For the sake of coverage, there are locations where it would take two failures to reach some lines of code. Your example of cleanup is a good one.
I think there are a few of potential solutions:
Setting potential alternate behaviors when the counter is negative, which is what you are suggesting. I think the idea works great, it would just require writing multiple unit tests for each behavior.
Instead of leaving it as -1, the
RCUTILS_SET_FAULT_COUNT
macro could have a variadic version which takes as parameters counts that it's reset to after it reaches 0.For example, would trip a second failure immediately after its first failure. This would let you test more types of failures, and then we could use this type of macro to check that code is not only one-fault tolerant, but two-fault tolerant, three-fault tolerant etc.
RCUTILS_CAN_RETURN_WITH_ERROR
, there would be aRCUTILS_CAN_RETURN_WITH_ERROR_NAMED
. Then in the unit test, we could target those locations to fail explicitly in a unit test.If we do introduce this one at a later date, I could see it replacing the unnamed version macro entirely. This idea requires multiple unit tests for each targeted failed injection, but is also the only option that allows for targeted injections in the first place.
Do you have any thoughts about the other options suitability for the situation you are describing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think solution (2.) would be best. We don't really know what we're hitting, so N-fault tolerant sounds as good as it gets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be added in a followup PR when the functionality becomes necessary for better coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main concern here is that we are going to litter our code with these macros. It's not the end of the world, but it is noisy and can make the code harder to read.
I have 2 separate suggestions:
LD_PRELOAD
mechanism to override the symbols as needed. This is less desirable than Mimick since it is limited to Linux-only, but I think we can make the case (at least for now) that doing it on one platform is better than doing it on zero platforms.The benefit to both solutions above is that there is no changes to the code, just changes to the tests. But I'd also like to hear what others think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed this PR last night, and intended to give a more full writeup. Please take a look at the above PR description for some more information as well as possible extensions.
The approach taken in this PR allows for immediate cross-platform, cross-language utilization with simple unit tests with no external dependencies. You can write a unit test that easily verifies your code is hardened against failures in upstream libraries. See ros2/rcl_logging#48 for an example.
Another approach was to add an
RCUTILS_CHECK_
macro that replaced simple if statements. Based on feedback from you and @wjwwood that suggested we want to be able to test the same code that we ship, I took this approach. The use of a macro likeRCUTILS_CAN_RETURN_WITH_ERROR_OF
allows for testing shipped production libraries against modified upstream libraries, which is not something our CI or buildfarm are setup for today but if desired could be feasibly updated in the future.I took care in the design of
RCUTILS_CAN_RETURN_WITH_ERROR_OF
to ensure that it wasn't just litter. It actually serves an additional purpose of describing intended failure cases of the function. While docblocks typically try to capture the possible failures cases, this would be another approach that can also be integrated with unit testing. It may be difficult for a maintainer to see all the possible return values of a high level function that it passes on from lower level functions. However, with unit tests utilizingRCUTILS_MAYBE_RETURN_ERROR
, you can verify that all possible return values are sufficiently understood.This PR is intended as a starting point, and I want to make it as useful as possible. There are several possible feature additions that I think could fill it out, but I would like to get some more buy in as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for using potentially harsh language above. That wasn't my intention.
As this stands, I think you've done a good job of minimizing the impact of these macros on the "live" code (that is, the real code we are trying to test and that we ship). That is, I don't see how this method of mocking could be any simpler than adding a single macro line to any function/method that you want to mock out. As we've discussed elsewhere, this also sets the stage for having the macro enabled for our nightly and CI builds, but disabled for our packaging and debian builds.
My biggest concern is deploying this kind of change across a large number of functions and methods throughout the ROS 2 codebase. Adding things like this to every function places additional mental burden on contributors, as all readers have to understand why they are here and what they do. It also makes it somewhat difficult to move away from this solution in the future if we find a better way.
I want to be clear that I am not totally against this change. However, I think some of the other options we are pursuing don't have the downsides of this solution, so I'd prefer to investigate those other options first. If those solutions turn out not to work for one reason or another, this is an acceptable path forward.
Does that clear up my thinking on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it does help clarify your thinking. My hope is that this sort of feature is used in conjunction with mocking like what @hidmic is investigating. That sort of testing would allow for more targeted failures, while this approach especially with the for loop example might be more like throwing a grenade at your code and seeing if it survives.
You're absolutely right that there are a large number of places where the
RCUTILS_CAN_RETURN_WITH_ERROR_OF
macro can be placed, and it would be overwhelming to put them in every single location at the outset, or even for every contribution. But I also think it's possible to add this macro in piecemeal, and we can start by only adding it in when it's needed for unit tests. For example, to push rcl_logging_spdlog to 100% coverage, you only needrcutils_snprintf
to fail, but given the correct inputs that function probably never will on our supported platforms.My suggestion with this sort of change is to start small and expand it as its role in this codebase is better understood. That's partially the reason why I only introduce one flavor of
RCUTILS_CAN_RETURN_WITH_ERROR_OF
, even though I also want a variadic version.That macro is empty if fault injection is not enabled, which I think helps make it easier to remove in the future. The example unit test in rcl_logging_spdlog would pass if
RCUTILS_CAN_RETURN_WITH_ERROR_OF
did not exist at all. We could also properly deprecateRCUTILS_SET_FAULT_INJECTION_COUNT
to help point external developers to the appropriate replacement.I also agree that seeing this macro at the top of a function would be strange and unusual to many developers, particularly newer ROS 2 users who are unfamiliar with the code. While I tried to be careful about the macro name, I'm also very open to rewording
RCUTILS_CAN_RETURN_WITH_ERROR_OF
. I chose the wording so that it's hopefully clear to developers unfamiliar with it that doesn't change the nominal behavior of the function, while conveying more information about the intent of the function's behavior if you are familiar with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clalancette I think this serves a different use case. Mocking is good for white-box unit testing, while this is performing black-box integration testing to see how the system, with all its side effects, behaves in an exceptional event (akin to exception safety verification). Could we use mocks to implement it? Yes, but we'd end up coupling a package to its dependencies' implementation and recreating side effects -- possible, but much harder.
IMHO,
RCUTILS_CAN_RETURN_WITH_ERROR_OF
, though atypical, isn't strictly worse than the prologue ofRCUTILS_CHECK_ARGUMENT_FOR_NULL
our C functions usually have.