std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.

[Jonathan Wakely]

On 06/11/20 22:13 +0100, Paul Scharnofske via Libstdc++ wrote:
>Disclaimer 1: That C++ Standard stuff seems really complicated, I only 
>really looked at
>https://cppreference.com and then verified it with 
>https://github.com/cplusplus/draft . I hope
>I got this right.
>
>Disclaimer 2: I've never send anything to a mailing list, I hope this works.
>
>The following snippet does the wrong thing:
>
>Â Â Â  #include <thread>
>
>Â Â Â  int main() {
>Â Â Â Â Â Â Â  std::jthread thread { [] {} };
>
>Â Â Â Â Â Â Â  // Calls std::terminate in std::thread::operator=(std::thread&&).
>Â Â Â Â Â Â Â  // Should instead join the previous thread before doing the 
>assignment.
>Â Â Â Â Â Â Â  thread = std::jthread{ [] {} };
>Â Â Â  }
>
>This looks like an oversight to me, from the standard:
>
>Â Â Â  32.4.4.2 Constructors, move, and assignment [thread.jthread.cons]
>
>Â Â Â  jthread& operator=(jthread&& x) noexcept;
>
>Â Â Â Â Â Â Â  Effects: If joinable() is true, calls request_stop() and then 
>join().
>Â Â Â Â Â Â Â  Assigns the state of x to *this and sets x to a default constructed
>Â Â Â Â Â Â Â  state. [...]
>
>Â Â Â  [...]
>
>Â Â Â  32.4.3.5 Assignment [thread.thread.assign]
>
>Â Â Â  thread& operator=(thread&& x) noexcept;
>
>Â Â Â Â Â Â Â  Effects: If joinable(), invokes terminate (14.6.2). Otherwise, 
>assigns
>Â Â Â Â Â Â Â  the state of x to *this and sets x to a default constructed state.
>Â Â Â Â Â Â Â  [...]
>
>I suggest the following fix: (mirror: 
>https://static.asynts.com/2020/11/06/jthread.patch)
>
>Â Â Â  diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>Â Â Â  index 89f9f6c8c38..02e4c3cc8a6 100644
>Â Â Â  --- a/libstdc++-v3/ChangeLog
>Â Â Â  +++ b/libstdc++-v3/ChangeLog
>Â Â Â  @@ -1,3 +1,8 @@
>    +2020-11-06  Paul Scharnofske  <asynts@gmail.com>
>Â Â Â  +
>Â Â Â  +Â Â Â Â Â Â  * include/std/thread (operator=(std::jthread&&): Join 
>current thread if it
>Â Â Â  +Â Â Â Â Â Â  is running before moving it.
>Â Â Â  +

The ChangeLog file is autogenerated from the Git commit logs, please
don't change it in patches (just provide the suggested commit log
message).

>     2020-11-05  Marek Polacek  <polacek@redhat.com>
>
>Â Â Â Â Â Â Â Â Â Â Â  PR c++/25814
>Â Â Â  diff --git a/libstdc++-v3/include/std/thread 
>b/libstdc++-v3/include/std/thread
>Â Â Â  index 887ee579962..773befa75ad 100644
>Â Â Â  --- a/libstdc++-v3/include/std/thread
>Â Â Â  +++ b/libstdc++-v3/include/std/thread
>Â Â Â  @@ -456,7 +456,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>Â Â Â Â Â Â Â Â  operator=(const jthread&) = delete;
>
>Â Â Â Â Â Â Â Â  jthread&
>Â Â Â  -Â Â Â  operator=(jthread&&) noexcept = default;
>Â Â Â  +Â Â Â  operator=(jthread&& __other) noexcept
>Â Â Â  +Â Â Â  {
>Â Â Â  +Â Â Â Â Â  if (joinable())
>Â Â Â  +Â Â Â Â Â Â Â  {
>Â Â Â  +Â Â Â Â Â Â Â Â Â  request_stop();
>Â Â Â  +
>Â Â Â  +Â Â Â Â Â Â Â Â Â  // The C++ Standard (working draft) says that this 
>method must be
>Â Â Â  +Â Â Â Â Â Â Â Â Â  // noexcept, but also dictates that join be called. It 
>doesn't say
>Â Â Â  +Â Â Â Â Â Â Â Â Â  // how to do this, this is probably the way to go?
>Â Â Â  +Â Â Â Â Â Â Â Â Â  try
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â  {
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â Â Â  join();
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â  }
>Â Â Â  +Â Â Â Â Â Â Â Â Â  catch (...)
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â  {
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â Â Â  std::terminate();
>Â Â Â  +Â Â Â Â Â Â Â Â Â Â Â  }
>Â Â Â  +Â Â Â Â Â Â Â  }
>Â Â Â  +
>Â Â Â  +Â Â Â Â Â  swap(__other);

Using swap doesn't seem to meet the requirement that __other is set to
a default constructed state, because __other._M_stop_state will still
have a shared state.

I think this would work:

   jthread& operator=(jthread&& __x) noexcept
   {
     std::jthread(std::move(__x)).swap(*this);
     return *this;
   }

It would modify __x before calling request_stop() and join(), which is
potentially observable from the thread being joined. But since this is
modifying __x, it's a data race for the other thread to access it
concurrently, which would have undefined behaviour. So I think it's
OK.


>Â Â Â  +
>Â Â Â  +Â Â Â Â Â  return *this;
>Â Â Â  +Â Â Â  }
>
>Â Â Â Â Â Â Â Â  void
>Â Â Â Â Â Â Â Â  swap(jthread& __other) noexcept
>
>Like already mentioned in the comment, there is a bit of an inconsistency in
>the Standard since the move assignment operator is marked as noexcept 
>and the join
>function can throw an exception...
>
>I don't know if there is something in the standard that dictates what 
>should happen in such
>a case, but I really don't know where to look for it.
>