support for multi-threaded test-cases?

[Kosta-Github]

Hey Phil,

do you have plans for supporting test-cases which internally spawn several threads to check correct behavior under multi-threaded conditions? This is not about executing several test cases in parallel.

At least the Junit reporter does not support that very well and crashes; the Console reporter doesn't seem to have that issue for the same tests (at the moment) but the absence of any sync'ing mechanisms let me worry about that this could change anytime soon...

For me it was enough to create a ThreadSafeJunitReporter by deriving from JunitReporter and adding a mutex/lock to the method assertionEnded() just like this:

class ThreadSafeJunitReporter : public Catch::JunitReporter {
public:
    ThreadSafeJunitReporter(Catch::ReporterConfig const& _config) :
        Catch::JunitReporter(_config) { }

    static std::string getDescription() {
        return "Reports test results in an XML format that looks like Ant's junitreport target.\n"
            "\tThis reporter can be used in a multi-threaded environment";
    }

    virtual bool assertionEnded(Catch::AssertionStats const& assertionStats) override {
        std::lock_guard<std::mutex> lock(m_mutex);
        return Catch::JunitReporter::assertionEnded(assertionStats);
    }

private:
    std::mutex m_mutex;
};

INTERNAL_CATCH_REGISTER_REPORTER("junit-thread-safe", ThreadSafeJunitReporter);

And using the command line option -r junit-thread-safe to get stable test runs again.

---

BTW: could you change this impl:

        void ReporterRegistry::registerReporter( std::string const& name, IReporterFactory* factory ) {
            m_factories.insert( std::make_pair( name, factory ) );
        }

into this one:

        void ReporterRegistry::registerReporter( std::string const& name, IReporterFactory* factory ) {
            m_factories[name] = factory;
        }

This would allow someone to replace an existing reporter with an own implementation. The insert() call above just inserts the given factory iff there is no factory with name registered yet. The operator[] call will insert or update m_factories in any case.

[philsquared]

Hi Kosta,

There's a few things in there.

1. I do have some long term plans around thread safety. They're tricky due to the combination of my requirements for no external dependencies and relying on C++98 only.
2. Getting the reporters to work is one thing, and kudos for getting a thread-safe JUnit reporter working. However there is other shared mutable state that will be just waiting for a clash, so I fear you're living on borrowed time there.
3. I have used Catch to test heavily threaded code. There's a simple workaround that can be made to work for a lot of cases (with a little effort). The basic principle is to simply wait until you're back on one thread (typically after a join) before you start asserting. If that doesn't drop out naturally (and I have a couple of cases where it doesn't) I've added extra code in my test to record the results of certain checks, then stuff it all in an INFO after the join and do a single assert on the whole result.
4. I'm not sure if I like the idea of overriding the existing reporter names with new behaviours. Is there a compelling reason to prefer that over a distinct name? If I did add support I would prefer to make it explicit when you register. There could also be evaluation order issues, since the registration relies on global initialiser instances.

[Kosta-Github]

Hey Phil,

1. I know you want to stick to (ancient) C++98, that is why I haven't investigated a cleaner solution and a corresponding PR, since I certainly would just use std::mutex for that case...

2. It looks like that if I am just using CATCH_REQUIRE() or CATCH_CHECK() from within the other threads I should be relatively safe with respect to manipulating shared state, at least for the moment; but I will have a closer look next week...

3. Sure, there are ways to transfer the test results out from the worker threads back into the master thread, but all this make the tests way more bloated and less readable; also the reasoning about what gets really tested by such a test case gets very hard. That is why I would like to have the ability to use at least both testing macros mentioned above from within separate threads; nothing fancy like multi-threaded TEST_SECTIONs or so... :-)

4. The overwriting of an existing reporter would just come in handy for the above case, in which I still could request the Junit reporter from the command line, but would provide the thread-safe one instead. Ordering of the registartion shouldn't be an issue in that/my case, since I would just register the thread-safe reporter directly from within main() and would not rely on static initialization.

[PureAbstract]

I know tou want to stick to (ancient) C++98

'Ancient' it may be (although I don't know what that makes me) - but it's still very widely used in real-world production systems.

[ned14]

For those needing a brutal hack job of a partially thread safe CATCH and are reading this issue, you may find my fork at https://github.com/ned14/Catch-ThreadSafe of use. I make no claims as to its reliability, but it works for me.

[hduden]

Hi Phil,

can you share anything about your "long term plans" for thread safety that you mentioned in your reply? I might be willing to contribute.

The Catch-ThreadSafe fork created by ned14 seems to be a good starting point, but you seem to think that more is required to get this water tight. My goal would be to not do this as another fork, because I would rather not have to sync with the main branch all the time. If you could outline what you think would be needed to get this accepted into the main branch then I will try to contribute a corresponding patch.

[ned14]

My thread safe fork has proven itself reliable under an awful lot of testing. However, it is a very nasty hack. I wouldn't do it that way in a million years if one had a choice.

[hduden]

Well, it depends on what the goal is. I think the most important thing is to get this feature into the main branch, because this is the main thing that is holding Catch back (IMHO). Multithreaded code is becoming increasingly common (especially since C++11 provides standardized support) and it is a shame to have Catch disqualified for a lot of bigger projects because of this. It is a hard sell to tell developers that they cannot use some features of the standard C++ library because of the testing framework that was chosen.

I honestly think that IF ned14s fork really has no problems (which I honestly cannot determine since I am too new to the inner workings of catch) then maybe it should simply be integrated into the main branch. Having better plans is nice, but the changes in the existing fork are relatively small and do not influence the public interface. Why not make integrate them as an option with a preprocessor switch (thus keeping C98 compatibility by default) and just integrate them to have the problem solved right now. If it works then there is no drawback that I can see. And the first implementation can easily be replaced later with a better one when Phil has time to devote to it.

If you (Phil) want a "better" first implementation using your own ideas, then let me repeat my earlier offer to help, if you would describe how you would like this to be done.

[ned14]

If one were to do a proper thread safe CATCH, you'd keep per-thread lock free results which are only collated and sorted into the correct order at the test end. Right now my hacked threadsafe CATCH is pretty useless for multithreaded testing because the mutex I'm using synchronises all the threads which ruins the test. I'm working around it with this idiom:

if(!(test))
CATCH_CHECK(test);

... which isn't ideal, but it solves my immediate problem.

[hduden]

Why is it necessary to aggregate per thread first? Could one not simply run the test and pass the result to a single shared RunContext object (as it is now), but simply protect that last result-storing operation with a mutex? Holding a mutex for storing the result should not be a big problem, performance-wise.
And from what I saw in the code, the state variables used during the actual test checks are all local (correct??). So it should not be necessary to protect those.

Naively it looks to me that one could probably remove the mutexes around the checks and instead just make the RunContext class thread-safe. I say naively because I am certain I am missing something vital here - or can it be that simple?

[hduden]

To clarify: by "making RunContext thread safe" I mean not just adding a few mutex locks. One would also have to convert some of the "lastXYZ" member variables to thread local ones. But I think it would be pretty straight-forward.

[ned14]

Why is it necessary to aggregate per thread first?

Multithreaded test cases are ruined by extra synchronisation. By per-thread lockfree results I specifically meant thread local storage. Note thread local storage (as in the C++11 thread_local) is profoundly broken on clang on OS X and FreeBSD right now unfortunately.

[hduden]

The compiler/stdlib feature for thread-local is not supported by Apple's clang, that is correct. But pthread_key_create will work on Mac and BSD. This could also be a fallback for cases when C98 compatibility is needed. So I think one should:

• use std::thread_local when C98 compatibility is not needed and we are on a platform that supports it.
• Fallback for the Unixes (Mac, BSD) should be pthread_key_create
• As the fallback for Windows (only needed in case of C98 compatibility) one should use TlsAlloc, because the MS-specific __declspec(thread) does not always work correctly when using DLLs.

So all in all one would have code that works on all C++11 compliant build systems and for C98 systems this would work for pthread-Unixes and Windows. I think that covers everything pretty well.

Only a few lines of code would be needed for each of the three implementations, so I think this is not a big addition.

So, assuming that we have working thread local storage: what else would be required, other than making RunContext safe?

After reviewing the code I think that we do not need the reporters to be thread-safe, since when they are called before tests start and after they end this is all from the main thread and no test cases (i.e. other threads) are running at the time. During tests they are only called through the RunContext, which we can modify so that it mutexes the accesses.
Do you agree with the assessment that the reporters can stay as-is?

[hduden]

Another option would be to not actually do a "real" thread local storage implementation for the different platforms. That does seem a little out of place for a testing suite, I have to admit.

Instead one could have a map or hash table in RunContext, mapping thread IDs to the thread state. This is very simple to implement and would only require a single platform dependent call for obtaining the thread id. It also makes cleaning up (and possibly analyzing) the thread states at the end of the test much easier.

Performance-wise, in a single threaded test, looking up the value from a single entry map is super fast and should not matter - it is barely more than a pointer dereferencing. Even with hundreds of threads in a single test, the overhead should not be a big problem. And after each test one could clean up the "thread-local" states, so it cannot degenerate with huge test suites.

I think I have convinced myself that this is a better solution for Catch. It reduces outside dependencies and platform-dependent code to a minimum. It would also make it pretty easy for a Catch user to provide his own getThreadID function if he is on an exotic platform that is not yet supported by the library.

[philsquared]

@ned14 could you elaborate on the "[tls] is profoundly broken on clang on OS X and FreeBSD right now"?

[ned14]

@philsquared I was referring exclusively to the C++ 11 thread_local storage attribute. The libc on OS X and BSD doesn't implement the on thread exit registration using C++ 11 semantics yet due to race problems during shared library unload.

You can still use __thread of course to use C semantics. Or a dense hash map indexed by thread id. Fastest would be the latter with the bucket cached into a TLS for fast access.

[philsquared]

Thanks Niall. That's a shame.
My intention was to use c++11 features (conditionally). I really didn't want to provide a whole platform abstraction over threads. Just TLS on Posix might be doable - or at least falling back to locking on every assertion (which is not ideal, I know, but at least gets Catch in the game).
I might also be able to do it using atomics.

[philsquared]

... of course if I finished my persistent hash-trie project I might be able to use that too ;-)

[rollbear]

..hash-tnie, eh? superficially combining orthogonal ideas, but I"m sure you have a clever spin on it. Care to share? I may let you in early on heap of mini-heaps, but I fear I digress...

[ned14]

Persistent hash-trie? There have been repeated attempts to get one of those into Boost as a prelude to entering the C++ standard library. Everybody to date has failed to make much progress ... and this is stretched over seven years now or something ... could I tempt you into submitting that for Boost?

[ned14]

Oh, C semantics TLS works fine on all compilers, including MSVC, and has done so for years. It's just the C++ destructors won't fire unless you do it by hand. thread_local storage does fire destructors for you, it's very handy - except on OS X and BSD to date.

[blastrock]

Hi,
Anything new on this issue? This is really a show-stopper for me :(

[philsquared]

I have made a start on a local branch, but it's been on hold for a while (too many balls in the air at the moment).
Definitely not lost, though.

[ned14]

If you're anything like me Phil, that ACCU conference sucks up all your free time from about February onwards! :)

[htfy96]

Hi,
Would it be better to mention thread-safety in the reference? Just spent hours on weird test failure before I found this issue 😓

[horenmar]

I am going to update documentation to mention that this is not supported and is unlikely to become supported until Catch 2, and close this with a revisit tag.

[adzenith]

Looks like there's more conversation about this in #1043.