Boost C++ Libraries: Ticket #9355: boost::coroutine crash in base::pull_coroutine_base with multiple threads

attachment set

craig@… — Thu, 07 Nov 2013 13:31:31 GMT

attachment → UnitTest_Concurrency_Test.cpp

coroutine multi-thread crash test code

attachment set

craig@… — Thu, 07 Nov 2013 13:32:26 GMT

attachment → run_concurrency.bat

Batch file to execute test continuously

olli — Thu, 07 Nov 2013 14:30:56 GMT

please reduce test code - it contains too much of your application (Jobs etc.)

attachment set

Thu, 07 Nov 2013 16:01:57 GMT

attachment → 2013-11-07 Boost Coroutine Concurrency Bug-Test.7z

Reduced test example with vc2012 solution

Thu, 07 Nov 2013 16:05:45 GMT

Thanks for the quick reply. I am very sorry for not cleaning up the original code example. I have now significantly reduced the code in a new attachment including a vc2012 project file. Please let me know if you would like me to reduce code further.

status changed; resolution set

olli — Thu, 07 Nov 2013 19:15:09 GMT

status new → closed
resolution → invalid

I've only msvc-10.0 available on my computer - your code does not compile with msvc-10.0.

On Linux + g++-4.7.3 you app asserts: ConcurrencyTest.cpp:163: void test(): Assertion `iterCounter == iterationCountTotal' failed. After commenting out the assert-statement the output is: Iters = 1024 of 1516918 [FALSE] Time: 107.453ms (iteration=70ns) Done it seams that you application logic is wrong

Your test app is still too complex - I need a small test app which triggers the error.

Fri, 08 Nov 2013 07:24:54 GMT

Very sorry about that, the asserts are left over from the original full code and apologize and have nothing to do with the test.

Your output sounds right. The coroutine is only constructed therefore performing just the first iteration in each thread as this is the part that causes the issue. I assume you have tested running an optimized GCC build without the debugger with no issue?

Fri, 08 Nov 2013 08:20:19 GMT

I have simplified down the test further removing all the logic and the use of vectors etc. You are right in that most of it is unrelated to the crashing and the issue still occurs.

Afterward is a new batch script too I have seen runs of ~100 without fail but have also seen the lucky crash inside the debugger but its the same as attaching after failure.

Test Code:

#include <boost/coroutine/all.hpp>
#include <boost/lockfree/queue.hpp>
#include <boost/bind.hpp>
#include <thread>
#include <queue>
typedef boost::coroutines::coroutine< void > coro_t;
static void foo( coro_t::push_type& yield, int i )
{}
struct Worker
{
	Worker() : done(false) {}
	volatile bool done;
	void operator()()
	{
		while ( !done )
		{
			pending.consume_all( [&]( int i )
			{
				coro_t::pull_type( boost::bind( foo, _1, i ) );
			} );
		}
	}
	boost::lockfree::queue<int, boost::lockfree::capacity<1024> > pending;
};
int main( int argc, char * argv[])
{
	Worker workers[2];
	std::thread threads[2] = { std::thread(std::ref(workers[0])) , std::thread(std::ref(workers[1])) };
	workers[0].pending.push(0);
	workers[1].pending.push(1);
	for ( auto& worker: workers )
		worker.done = true;
	for ( auto& thread: threads )
		thread.join();
	return EXIT_SUCCESS;
}

Batch script:

echo off
cls
set counter=0
:loop
set /a counter=counter+1
echo run %counter%
x64\Release\ConcurrencyTest.exe
goto loop

status changed; resolution deleted

Fri, 08 Nov 2013 08:36:44 GMT

status closed → reopened
resolution invalid

status changed; resolution set

olli — Fri, 08 Nov 2013 18:36:11 GMT

status reopened → closed
resolution → worksforme

I can't reproduce your problem - I've tested the code (a little bit modified) on LINUX 32/64bit gcc-4.7.3 debug/release as well as on Windows7 with MSVC-10.0 32/64bit debug/release.

attachment set

olli — Fri, 08 Nov 2013 18:37:17 GMT

attachment → test.cpp

craig@… — Fri, 08 Nov 2013 20:42:40 GMT

Thank you for looking into the issue and know your time is probably precious. Your modified code does not produce the problem as you have identified. The batch script was used for a reason that I should have made clear. The issue occurs every N runs of the application. Be it some optimization or the likes but performing a loop like you have added does not cause the crash. Its a first time sort of issue annoyingly so tricky to track.

olli — Fri, 08 Nov 2013 21:31:10 GMT

I've also used the batch script - after the 555 invocation of the test-app (which itself runs your test code in a loop 1000x) I did not get a crash. Please not that I've only MSVC-10.0 - maybe yo should verify the code with MSVC-10.0 on your system.

craig@… — Fri, 08 Nov 2013 21:49:36 GMT

I have tested the issue under vc2010 on a different system and can also reproduce the issue. This system has a Intel Centrino 2 core and does not experience the crash when only 2 threads are utilized. With 4 threads the issue however occurs but due to whatever the cause may be this processor will crash between 580-1120 executions of the program so is not a good platform to find or test on. I will try and test an i5 4670 at the weekend and see if it similar to the i7-3960 of the original system.

New code:

#include <queue>
#include <boost/atomic.hpp>
#include <boost/bind.hpp>
#include <boost/coroutine/all.hpp>
#include <boost/lockfree/queue.hpp>
#include <boost/thread.hpp>
typedef boost::coroutines::coroutine< void > coro_t;
static void foo( coro_t::push_type& yield, int i )
{}
struct Worker
{
    boost::atomic< bool > done;
    Worker() :
        done( false)
    {}
    void operator()()
    {
        while ( ! done)
        {
            pending.consume_all( [&]( int i)
            {
                coro_t::pull_type( boost::bind( foo, _1, i) );
            } );
        }
    }
    boost::lockfree::queue< int, boost::lockfree::capacity< 1024 > >    pending;
};
int main( int argc, char * argv[])
{
	const uint32_t kCount = 4;
    Worker workers[kCount];
    boost::thread threads[kCount];
	for ( uint32_t i = 0; i < kCount; ++i )
		threads[i] = boost::thread( boost::ref( workers[i]) );
	for ( uint32_t i = 0; i < kCount; ++i )
        workers[i].pending.push(i);
	for ( uint32_t i = 0; i < kCount; ++i )
        workers[i].done = true;
	for ( uint32_t i = 0; i < kCount; ++i )
        threads[i].join();
    return EXIT_SUCCESS;
}

Sat, 09 Nov 2013 18:07:46 GMT

Okay so I have tested a Haswell i5-4670 and with 2 thread the issue too 53,161 executions to occur, however up the test to 4 threads and the crash occurs every 2-4 executions! I don't know that that really indicates as on the i7 two threads was causing the issue so could be amplified by hyper-threading somehow.

I don't like the 'fix' but putting a mutex lock around the constructor for the coroutine is the only work around I have currently found

{
boost::lock_guard<boost::recursive_mutex> lock(m_guard);
coro_t::pull_type( boost::bind( foo, _1, i) );
}

status changed; resolution deleted

Wed, 13 Nov 2013 08:17:04 GMT

status closed → reopened
resolution worksforme

I have been drilling down into the code and have located the offending problem in standard_stack_allocator_windows.cpp @ line 63:

SYSTEM_INFO system_info()
{
    static SYSTEM_INFO si = system_info_();
    return si;
}

This function is called in standard_stack_allocator::allocate(...) via detail::pagesize(). The issue here is the use of a static variable in an unsafe manner between threads which explains why it might only fall over the first time and then run without fail.

The first thread is initializing the static value and writing to the structure. A second thread is then already reading/read data from it. This means pagesize() is returning an uninitialized variable.

I don't know the performance characteristics of GetSystemInfo() but a very simple work-around for the crashes is to remove the 'static' keyword:

/*static*/ SYSTEM_INFO si = system_info_();

A better solution is a critical section though as GetSystemInfo() may take an unkown time which I think is the root to the issue to begin with.

Wed, 13 Nov 2013 08:21:34 GMT

I think this answers the issue and its a compiler problem of sorts and fact this person reports most compilers still don't implement static variable concurrency safely: http://stackoverflow.com/a/4590634

Wed, 13 Nov 2013 08:48:05 GMT

A mutex based solution would be:

SYSTEM_INFO system_info()
{
    boost::lock_guard<boost::recursive_mutex> lock(m_guard);
    static SYSTEM_INFO si = system_info_();
    return si;
}

This isn't optimal though as there should be a way to only block on first entry to the function somehow.

Wed, 13 Nov 2013 09:11:38 GMT

A simpler solution is to put the static into global scope it would appear so that the data is obtained prior to co-routine constructors. It works, and I think to get a race condition on a global static isn't really ever going to happen unless somebody created lots of global static threads too which would be sort of silly.

Wed, 13 Nov 2013 09:33:38 GMT

Okay so I have learnt lots of new things today. I have a pretty good solution wihtout the performance issue of a mutex. I think boost probably has a call_once too but used std in this test:

#include <mutex>
...
static std::once_flag l_onceFlag;
SYSTEM_INFO system_info()
{
    static SYSTEM_INFO si;
	 std::call_once(l_onceFlag,  [&]
	 {
		 si = system_info_();
	 });
    return si;
}

status changed; resolution set

olli — Wed, 13 Nov 2013 16:33:50 GMT

status reopened → closed
resolution → fixed

thx, the bug is fixed in boost-trunk, please verify