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

Section: 32.10.6 [futures.promise], 32.10.10.2 [futures.task.members] Status: C++20 Submitter: Jonathan Wakely Opened: 2014-06-23 Last modified: 2021-02-25

Priority: 3

View other active issues in [futures.promise].

View all other issues in [futures.promise].

View all issues with C++20 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 32.10.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.9.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 32.10.10.2 [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.9.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.

[2018-06, Rapperswil, Wednesday evening session]

JW: lets move on and I'll file another issue to make the wording better
BO: the current wording is better than what there before
JM: ACTION I should file an editorial issue to clean up on how to refer to [res.on.data.races]: raised editorial issue 2097
ACTION: move to Ready

Daniel rebases wording to N4750.

[2018-11, Adopted in San Diego]

Proposed resolution:

This wording is relative to N4750.

1. Change 32.10.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.9.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 32.10.10.2 [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.9.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: […]