Possible race in std::packaged_task

labath commented 5 years ago


Bugzilla Link	43228
Version	unspecified
OS	All
CC	@mclow

Extended Description

Running the following piece of code under tsan:

  auto task = std::make_shared<std::packaged_task<void()>>(
      [] { std::cout << "XXX" << std::endl; });
  std::thread([task] { (*task)(); }).detach();
  task->get_future().wait();

Gives the following error message:

  Read of size 4 at 0x7b200000c2f0 by thread T5:
    #0 std::__1::__assoc_sub_state::__has_value() const include/c++/v1/future:551:18
    #1 std::__1::packaged_task<void ()>::operator()() include/c++/v1/future:2207:24
    #2 main()::$_1::operator()() const
    #3 ...

  Previous write of size 4 at 0x7b200000c2f0 by main thread (mutexes: write M2410):
    #0 std::__1::__assoc_sub_state::__attach_future() include/c++/v1/future:560:18
    #1 std::__1::future<void>::future(std::__tsan::__assoc_sub_state*) llvm/projects/libcxx/src/future.cpp:181:15
    #2 std::__1::promise<void>::get_future() llvm/projects/libcxx/src/future.cpp:223:12
    #3 std::__1::packaged_task<void ()>::get_future() include/c++/v1/future:2192:51
    #4 main()
    #5 ...

Looking at the code, once of the threads is doing return __state_ & __constructed, and the other one does a __state_ |= __future_attached.

Now, I am not completely sure that what this snippet of code (obtained by reduction from some code in LLDB) is doing is absolutely correct. I have looked at the standard, and I could not find evidence either way (though it's possible I am just not looking at the right place). However, there are some hints which lead me to believe that this might not be intentional: a) this code is "almost" race free. The only reason we are getting an error is because the two threads are accessing different bits in the same integer. If these were two integers everything would be ok. b) cppreference.com https://en.cppreference.com/w/cpp/thread/promise/get_future explicitly says that promise::get_future does not race with promise::set_value. That said, I am not sure what is the basis of this claim of their, and also cppreference is suspiciously quiet about the packaged_task class.

I'd appreciate it if someone with more experience experience reading the c++ standard could take a look at this and confirm whether this is a libc++ bug, or an lldb bug.

jwakely commented 1 week ago

https://en.cppreference.com/w/cpp/thread/promise/get_future explicitly says that promise::get_future does not race with promise::set_value.

packaged_task::get_future() clearly says it doesn't race with operator():

Synchronization: Calls to this function do not introduce data races (6.9.2) with calls to operator() or make_ready_at_thread_exit.

Libc++ happens to implement this in terms of std::promise, but that shouldn't matter: packaged_task::get_future() must not introduce data races when called concurrently with making the state ready.

As it happens, std::promise::get_future() has the same guarantee:

Synchronization: Calls to this function do not introduce data races (6.9.2) with calls to set_value, set_exception, set_value_at_thread_exit, or set_exception_at_thread_exit.

But that's not really relevant here, since the use of std::promise in std::packaged_task is an implementation detail, and packaged_task says it needs to work.

jwakely commented 1 week ago

(it would be nice if somebody could edit the OP so it looks more like https://bugs.llvm.org/show_bug.cgi?id=43228 instead of the formatting being butchered)

llvm / llvm-project

Possible race in std::packaged_task #42573

Extended Description