[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:
Backports
Depends
is depended on by SERVER-51821 Always join threads in SharedFuture t... Closed
Related
related to SERVER-54531 All ASSERTs should fail hard if they'... Closed
related to SERVER-53065 std::terminate handler must learn to ... Closed
is related to SERVER-53049 stdx::condition invariants and segfau... Closed
Backwards Compatibility: Fully Compatible
Backport Requested:
v4.4
Sprint: Service arch 2020-11-30
Participants:
Linked BF Score: 28

 Description   

Investigating SERVER-51821, I found an ASSERT_EQ in a worker thread, which, if triggered, would crash the process with std::terminate. It is not valid to perform any kind of ASSERT from unit test thread other than the main thread.

This cause of std::terminate is NOT ultimately the symptom of SERVER-51821, in which the controlling thread simply stopped (via throw) and never executed the explicit .join of the worker threads at the bottom of the test. But investigation of SERVER-51821 uncovered the ASSERT from non-main problem to be a very common weakness that exists in most of our concurrent unit tests.

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: SERVER-53035 ThreadAssertionMonitor
Branch: v4.4
https://github.com/mongodb/mongo/commit/de32d95a395f60bbea3d9c6f39cc1285fac4a41b

Comment by Billy Donahue [ 24/Nov/20 ]

Closing as we have the ASSERT transporting system in place,
as unittest::ThreadAssertMonitor.

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: SERVER-53035 ThreadAssertionMonitor
Branch: master
https://github.com/mongodb/mongo/commit/5e82f974f44a822753ad0656692a1011a6611b3c

Comment by Billy Donahue [ 24/Nov/20 ]

This PoC adds an invariant if ASSERT comparisons happen outside a TEST thread.
https://github.com/BillyDonahue/mongo/commit/25d2315a95943019b9219dfa51861bf46bd88b9b
I'll be curious to see what it turns up.

https://spruce.mongodb.com/version/5fbc796657e85a38e7937d4f/tasks

A lot of tests failed. Several failed that were actually fine.
Many of the failures were false positives from tests using std::async and std::future::get in the main thread. std::future transports exceptions to the .get() caller properly so these are not a problem even though they are technically ASSERT statements that are outside of the main thread.

Several such tests are just using stdx::future to get more work done in parallel.
I believe several tests were parallelized this way a while back.

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.
https://spruce.mongodb.com/version/5fbd1fd4d1fe072958c0e9cc/tasks

Comment by Billy Donahue [ 23/Nov/20 ]

Code Review: https://mongodbcr.appspot.com/720450002/

Generated at Thu Feb 08 05:29:44 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.