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

[Jonathan Wakely <jwakely@redhat.com>]

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.
>