blocking_get randomly does not return.

[FunMiles]

Trying to get acquainted with the concurrency library, I tried the following code and it randomly gets stuck, even though the tasks associated with the futures all have been completed. Sometimes the executable will finish, sometimes it will not, staying stuck in one of the blocking_get(f).

#include <iostream>
#include <thread>
#include <stlab/concurrency/concurrency.hpp>
using namespace std;
using namespace stlab;

int main() {
	std::vector<future<int>> allFutures;
	auto &the_executor = default_executor;
	for(int i = 0; i < 32; ++i)
		allFutures.push_back(async(the_executor, [](int i) {
			for(int j = 0; j < 16'000'000; ++j)
				i *= (2*j+1);
			std::cout << "Got " << i << std::endl;
			return i;
		}, i));
	for(auto &f : allFutures)
		std::cout << "R: " << blocking_get(f) << std::endl;
        return 0;
}

Running on Mac OS X 10.12.6. Compiled with clang++ and C++17.

[FelixPetriconi]

So far this is not a bug in the library, but a problem of the code. In the way as you call blocking get on all the futures, you can come into a dead lock situation on the thread pool. Sean Parent goes in detail into this problem in his talk: https://www.youtube.com/watch?v=QIHy8pXbneI at timestamp 1:18:00

The general recommendation is: Use never blocking_get! Use continuations wherever possible. I use blocking_get() only in test applications to ensure, that a single, final calculation is done and the program does not end, before that value is calculated.

I recommend to change the code into the following by using the joining operation of when_all().

#include
#include
#include <stlab/concurrency/concurrency.hpp>

using namespace std;
using namespace stlab;

int main() {
std::vector<future> allFutures;

for(int i = 0; i < 32; ++i)
	allFutures.push_back(async(default_executor, [](int i) {
		for(int j = 0; j < 16'000'000; ++j)
			i *= (2*j+1);
		std::cout << "Got " << i << std::endl;
		return i;
	}, i));

auto end = when_all(default_executor, [](auto&& results) {
	for (const auto& r:  results)
		cout << "R: " << r << "\n";
}, std::make_pair(allFutures.begin(), allFutures.end()));

blocking_get(end);

return 0;
}

[FunMiles]

I disagree.

• Mac OS X is not single threaded (the video, at 1:18 talks about being able to bring it down to 1 thread).
• Even in a single threaded system, each 'blocking_get' should be calling the task's lambda and there should not be a deadlock.
• The typical output when it gets stuck has all the "Got" 32 times. So all the tasks have executed and finished. For a deadlock, you need that two parts wait for the other to take action. Yet the task on whose future the blocking_get does not return has finished. The task contains no lock whatsoever.

I had tested the when_all before doing this more simple test. I know it works, but I wanted to do different tests. I still think this is a bug.

[sean-parent]

I believe you are correct @FunMiles that this example is a bug. I'll try and investigate further this evening.

However, we currently do not do task-promotion (immediate execution of the task on a get()). Task promotion can only be done for tasks that are scheduled on a non-serialized executor, and have no other tasks upon which they depend, unless all of those tasks can be promoted (for example, a task associated with a continuation has to also promote the task it is continuing). This is one reason why we don't have a generalized .get() but instead provide a blocking_get(). In order to use a blocking get (or wait) safely you have to know about the tasks that provide data to the future and we are trying to avoid systems that require non-local reasoning. We may add some form of promotion in the future, but only if we can do so without cost or through a more explicit API (make_promotable...).

[sean-parent]

Status update -
This bug is very reproducible. Nothing shows up running thread sanitizer (except a race using cout but not calling cout from the thread fixes that without changing the hang).

We hang waiting on the condition variable inside blocking_get() except the recover continuation has already fired and set == true. Which makes zero sense. I stepped through the assembly for the code and it looks like the set = true in the lambda is correctly executing under the lock, there is appropriate memory barriers, and the notify_one() executes. The other side for the wait() looks equally legit. However, by putting in an atomic counters I have verified that the hang occurs when the loop is entered simultaneously with the completion executing - which would indicate a race but I'm not seeing where.

I've tried a few shots in the dark, setting set to true through a pointer instead of a reference, making set an atomic (in case it was a bug with the memory barrier logic in the compiler) but it doesn't change the behavior. Still looking but at the moment I'm stumped...

[sean-parent]

Another odd data point - pausing in the debugger when it is stuck, and then continuing, will cause it to complete successfully (might cause a "spurious" wake on the condition variable - but still odd).

[sean-parent]

Ahh... I believe I see the problem.

After the lambda sets the flag to true, the function may exit before the condition variable is notified. In which case we notify a dead condition variable. However, because we are looping I think we end up notifying the condition variable that lands on the stack the next time around and then we are in a bad state... Looking into fixes.

[sean-parent]

Okay - that is going to be a good example for a talk... moving the notify_one() call under the lock fixes the issue. This is normally a pessimization (but with well defined behavior) because the code calling wait() will wake but immediately block re-acquiring the mutex. However, calling notify_one() from under the lock ensures the function can't exit (or destruct the condition variable) before notify_one() is called... I'll get a PR together in the morning... Wow... I've never hit that issue. Going to have to review my use of condition variables.

[FunMiles]

Great. Looking forward to pulling the fix.
@sean-parent, this makes your 'no raw synchronization primitives' all the more relevant. I'm looking forward to your new talk.

Michel Lesoinne

[FelixPetriconi]

Fixed in 1.2.0