TSAN: data race related to hasLoggedCurrentTestStart #217
Comments
|
hopefully the subcases in the test case are all traversed from a single thread...? So if you spawn multiple threads - they all have to join before another subcase is entered or the current one is exited. This sounds serious and I'll look into it when I have the chance. |
Yes, that's the main thread. The other thread(s) end up eventually calling mocks from Trompeloeil, and the glue code (which integrates trompeloeil with doctest) calls "stuff in doctest", in this case,
I do not fully understand this. I hope you do not mean that I have to join all threads which might end up "calling into" doctest, such as shown here? I'm using quite some nesting of subsections... |
|
Some examples: this is safe: TEST_CASE("OK") {
SUBCASE("") {
CHECK(1 == 2);
}
std::thread t1([](){
CHECK(1 == 2);
});
std::thread t2([](){
CHECK(1 == 2);
});
CHECK(1 == 2);
t1.join();
t2.join();
SUBCASE("") {
CHECK(1 == 2);
}
}This is not: TEST_CASE("bad 1") {
std::thread t1([]() {
SUBCASE("t_sub") {}
});
SUBCASE("sub") {}
t1.join();
}
TEST_CASE("bad 2") {
std::thread t1([]() {
CHECK(1 == 2);
});
SUBCASE("sub") {}
t1.join();
}basically all threads that have been started within a subcase should end before it is exited, and no other subcases should be entered (this is more correct than my previous comment). I'm not even sure I'm explaining it properly - long day... I might look into relaxing these requirements. Btw I've seen many times people using subcases not to share setup/teardown code but to structure tests hierarchically and nest them - with doctest that can be done with test suites + test cases. |
|
We definitely use real-world code like this: TEST_CASE("device operations") {
// this creates the "io" variable which is a Trompeloeil mock
// it also uses a trompeloeil::sequence which calls REQUIRE(...) from its destructor
INIT_MOCKED_IO;
// this starts a background thread which uses "io" and therefore calls
// into Trompeloeil mock which in turn calls REQUIRE(a == b) from that background thread
ServiceWorker w(io);
DeviceUnderTest thing;
SUBCASE("device reset") {
REQUIRE_CALL(io, write("reset"));
thing.reset();
}
SUBCASE("poweroff") {
REQUIRE_CALL(io, write("poweroff"));
thing.poweroff();
}
REQUIRE(io.ok());
// ~ServiceWorker() gets called, which eventually joins its background thread
// And here, behind the scenes, trompeloeil::sequence::~sequence()
// performs additional checks and calls REQUIRE(...)
}To me, this looks like your "bad 2" test case even though I would classify it as "just sharing common startup and teardown". Do you think that this is not a reasonable setup? |
|
You are indeed correct that this is a bug according to what I've written in the FAQ about thread-safety - thanks for reporting this. I really could just put a lock around I have only 1 concern and it can be illustrated with EDIT: anyway I just pushed a fix so let me know if the issue persists. I guess that concern I have is ... dumb. There are already no guarantees about the order of assert execution in a multi-threaded context anyway... |
That's not a bug. For me, that's actually the expected behavior. The background thread is typically "somehow related" to the main thread which descends into subcases. In my case, it is a background worker which handles requests submitted by the subcases. If the error report only included info about when the background thread was started, that would actually mask valuable information. Thanks for fixing this. I'll give this a bunch of serial runs and report back if I see any more failures. I'll close this one as it indeed appears to be fixed. |
I'm on Fedora 29, clang 7.0.1, latest
devof doctest as of yesterday. It seems that there's a race when two threads accesshasLoggedCurrentTestStart, for example:doctest::(anonymous namespace)::ConsoleReporter::logTestStart()in thread A,doctest::(anonymous namespace)::ConsoleReporter::subcase_end()in thread B.In the report below, thread
T1is a background worker thread which calls to methods mocked via Trompeloeil. Everything is driven by a singleTEST_CASEmacro, so I believe, based on the thread safety promise listed in the FAQ, that this is a bug.I don't have an easy reproducer, unfortunately. I found this when looking for another problem, and it required running our internal test suite in a loop, sometimes only failing after 25+ iterations...
The text was updated successfully, but these errors were encountered: