[SERVER-53035] provide a way to ASSERT from a non-main thread in unit tests Created: 23/Nov/20 Updated: 29/Oct/23 Resolved: 24/Nov/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | 4.9.0, 4.4.7 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Billy Donahue | Assignee: | Billy Donahue |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||
| Backport Requested: |
v4.4
|
||||||||||||||||||||||||||||
| Sprint: | Service arch 2020-11-30 | ||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||
| Linked BF Score: | 28 | ||||||||||||||||||||||||||||
| Description |
|
Investigating This cause of std::terminate is NOT ultimately the symptom of I propose that we identify ASSERTs that need to be fixed by changing ASSERT so that even if it passes, it will fail unless it is running on a safe thread. |
| Comments |
| Comment by Githook User [ 21/May/21 ] |
|
Author: {'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}Message: |
| Comment by Billy Donahue [ 24/Nov/20 ] |
|
Closing as we have the ASSERT transporting system in place, Further work would be to identify and fix all the unsafe ASSERTs. In some sense it might be ok to leave them alone, because if a worker thread leaks an ASSERT exception and brings down the whole unit test, we will not mistake that for a passing test. It's just that other tests won't get a chance to run, and the cause of failure won't be clear.... It could be clear if the std::terminate handler we install in unit tests knew how to catch unitttest::TestAssertionFailureException to print it out on its way to std::abort, but that exception type is deliberately hard to catch. But std::terminate is special. We should add a hook for that. |
| Comment by Githook User [ 24/Nov/20 ] |
|
Author: {'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}Message: |
| Comment by Billy Donahue [ 24/Nov/20 ] |
|
This PoC adds an invariant if ASSERT comparisons happen outside a TEST thread. https://spruce.mongodb.com/version/5fbc796657e85a38e7937d4f/tasks A lot of tests failed. Several failed that were actually fine. Several such tests are just using stdx::future to get more work done in parallel. For those tests, we can turn off the check entirely, or add a scope guard to mark the current thread ok at the top of the async lambdas. Example: testPermutation in src/mongo/db/storage/key_string_test.cpp Another go at it with better diagnostic reporting. |
| Comment by Billy Donahue [ 23/Nov/20 ] |
|
Code Review: https://mongodbcr.appspot.com/720450002/ |