boost::promise runs arbitrary code while holding mutex lock

[Jonathan Wakely <jwakely.boost@…>]

This deadlocks or aborts, depending on how pthreads handles re-locking a mutex in the same thread:

#include <boost/thread/future.hpp>

struct X {
  X() { }
  X(const X&);
};

boost::unique_future<X> fut;

X::X(const X&) { fut.wait_for(boost::chrono::milliseconds(1)); }

int main()
{
  boost::promise<X> p;
  fut = p.get_future();
  p.set_value( X() );
}

promise::set_value() owns a lock while invoking the copy constructor, which might have side-effects.

[viboes]

This example seems a little bit contrived to me.

Should the library take care of this case? What the standard says? Have you tried it with some standard implementations?

[Jonathan Wakely <jwakely.boost@…>]

It's a bit contrived, yes, but this is reduced to the minimum needed to show it.

IMHO it's conceivable that the result's copy constructor would try to do something (e.g. logging or performance measurements) which might end up checking for readiness of something, which depends on the result we're initializing.

The standard doesn't say anything about this, which I interpret to mean it's supposed to work. Otherwise I would expect the standard to place preconditions on promise::set_value(). Since I interpret it that way, GCC does not deadlock for code like this (no lock is held which the result object is copied). libc++ either deadlocks or aborts depending on the Pthreads implementation (attempting to lock a mutex in the thread that already owns it is undefined behaviour).

[viboes]

How do you ensure that only one thread is doing the set_value without maintaining the lock?

[anonymous]

std::call_once

[viboes]

This would mean a severe refactoring. Doesn't this would mean a bottleneck on call_once?

I would need sometime to digest the change.

[jwakely.boost@…]

call_once adds a bit of overhead, but is only a bottleneck if multiple threads are trying to make the shared state ready at the same time, and since all but the first are going to fail with an exception containing promise_already_satisfied it doesn't really matter if they're blocked on call_once.

From memory, my design is roughly:

void
state::set(function<unique_ptr<Result>()> setter)
{
  unique_lock<mutex> l(m_mutex, defer_lock);
  call_once(m_once, &state::do_set, this, ref(setter), ref(l));
  if (!l.owns_lock())
    throw_promise_already_satisfied_err();
}

void
state::do_set(function<unique_ptr<Result>()>& setter, unique_lock<mutex>& l)
{
  auto result = setter(); // run without lock held
  l.lock();
  m_result.swap(result);  // non-blocking
}

Result is a wrapper for the result, containing exception_ptr and R for a future<R>, or exception_ptr and R* for future<R&>, or just exception_ptr for future<void>. The setter function creates a new Result and sets its members (possibly by running a deferred function or a packaged_task's callable) and any copy construction of R happens inside that setter, before the mutex is locked.

The mutex is locked to transfer the Result into m_result, and also locked by waiting functions checking whether m_result != nullptr.

I'm not sure that Boost's implementation should support my strange scenario - I'm still not certain my testcase is valid. No other implementations seem to support it, and maybe noone really cares if it works, so a severe refactoring probably isn't worth it.

[viboes]

Thanks for sharing this. I would close it for the time been as wontfix. We could reopen it if the case is found on user code.