Boost C++ Libraries: Ticket #10056: boost::promise runs arbitrary code while holding mutex lock

owner, status changed

viboes — Sun, 18 May 2014 18:35:52 GMT

owner changed from Anthony Williams to viboes
status new → assigned

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?

Mon, 19 May 2014 13:31:16 GMT

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 — Fri, 30 May 2014 20:57:24 GMT

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

anonymous — Fri, 30 May 2014 22:30:11 GMT

std::call_once

viboes — Fri, 30 May 2014 23:55:57 GMT

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@… — Sat, 31 May 2014 00:38:06 GMT

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.

status changed; resolution set; milestone deleted

viboes — Thu, 05 Jun 2014 21:46:05 GMT

status assigned → closed
resolution → wontfix
milestone To Be Determined

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.