2412. promise::set_value() and promise::get_future() should not race

Section: 33.6.6 [futures.promise], 33.6.10.1 [futures.task.members] Status: Review Submitter: Jonathan Wakely Opened: 2014-06-23 Last modified: 2017-03-01

Priority: Not Prioritized

View other active issues in [futures.promise].

View all other issues in [futures.promise].

View all issues with Review status.

Discussion:

The following code has a data race according to the standard:

std::promise<void> p;
std::thread t{ []{
  p.get_future().wait();
}};
p.set_value();
t.join();

The problem is that both promise::set_value() and promise::get_future() are non-const member functions which modify the same object, and we only have wording saying that the set_value() and wait() calls (i.e. calls setting and reading the shared state) are synchronized.

The calls don't actually access the same memory locations, so the standard should allow it. My suggestion is to state that calling get_future() does not conflict with calling the various functions that make the shared state ready, but clarify with a note that this does not imply any synchronization or "happens before", only being free from data races.

[2015-02 Cologne]

Handed over to SG1.

[2016-10-21, Nico comments]

After creating a promise or packaged task one thread can call get_future() while another thread can set values/exceptions (either directly or via function call). This happens very easily.

Consider:

promise<string> p;
thread t(doSomething, ref(p));
cout << "result: " << p.get_future().get() << endl;

AFAIK, this is currently UB due to a data race (calling get_future() for the promise might happen while setting the value in the promise).

Yes, a fix is pretty easy:

promise<string> p;
future<string> f(p.get_future());
thread t(doSomething, ref(p));
cout << "result: " << f.get() << endl;

but I would like to have get_future() and setters be synchronized to avoid this UB.

This would especially make the use of packaged tasks a lot easier. Consider:

vector<packaged_task<int(char)>> tasks;
packaged_task<int(char)> t1(func);

// start separate thread to run all tasks:
auto futCallTasks = async(launch::async, callTasks, ref(tasks));

for (auto& fut : tasksResults) {
  cout << "result: " << fut.get_future().get() << endl; // OOPS: UB
}

Again, AFAIK, this program currently is UB due to a data race. Instead, currently I'd have to program, which is a lot less convenient:

vector<packaged_task<int(char)>> tasks;
vector<future<int>> tasksResults;
packaged_task<int(char)> t1(func);
tasksResults.push_back(t1.getFuture()));
tasks.push_back(move(t1));

// start separate thread to run all tasks:
auto futCallTasks = async(launch::async, callTasks, ref(tasks));

for (auto& fut : tasksResults) {
  cout << "result: " << fut.get() << endl;
}

With my naive thinking I see not reason not to guarantee that these calls synchronize (as get_future returns an "address/reference" while all setters set the values there).

Previous resolution [SUPERSEDED]:

This wording is relative to N3936.

  1. Change 33.6.6 [futures.promise] around p12 as indicated:

    future<R> get_future();
    

    -12- Returns: A future<R> object with the same shared state as *this.

    -?- Synchronization: Calls to this function do not conflict (6.8.2 [intro.multithread]) with calls to set_value, set_exception, set_value_at_thread_exit, or set_exception_at_thread_exit. [Note: Such calls need not be synchronized, but implementations must ensure they do not introduce data races. — end note]

    -13- Throws: future_error if *this has no shared state or if get_future has already been called on a promise with the same shared state as *this.

    -14- Error conditions: […]

  2. Change 33.6.10.1 [futures.task.members] around p13 as indicated:

    future<R> get_future();
    

    -13- Returns: A future<R> object that shares the same shared state as *this.

    -?- Synchronization: Calls to this function do not conflict (6.8.2 [intro.multithread]) with calls to operator() or make_ready_at_thread_exit. [Note: Such calls need not be synchronized, but implementations must ensure they do not introduce data races. — end note]

    -14- Throws: a future_error object if an error occurs.

    -15- Error conditions: […]

[2017-02-28, Kona]

SG1 has updated wording for LWG 2412. SG1 voted to move this to Ready status by unanimous consent.

[2017-03-01, Kona, SG1]

GeoffR to forward revised wording.

Proposed resolution:

This wording is relative to N4640.

  1. Change 33.6.6 [futures.promise] around p12 as indicated:

    future<R> get_future();
    

    -12- Returns: A future<R> object with the same shared state as *this.

    -?- Synchronization: Calls to this function do not introduce data races (6.8.2 [intro.multithread]) with calls to set_value, set_exception, set_value_at_thread_exit, or set_exception_at_thread_exit. [Note: Such calls need not synchronize with each other. — end note]

    -13- Throws: future_error if *this has no shared state or if get_future has already been called on a promise with the same shared state as *this.

    -14- Error conditions: […]

  2. Change 33.6.10.1 [futures.task.members] around p13 as indicated:

    future<R> get_future();
    

    -13- Returns: A future object that shares the same shared state as *this.

    -?- Synchronization: Calls to this function do not introduce data races (6.8.2 [intro.multithread]) with calls to operator() or make_ready_at_thread_exit. [Note: Such calls need not synchronize with each other. — end note]

    -14- Throws: a future_error object if an error occurs.

    -15- Error conditions: […]