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

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

[Paul Scharnofske]

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.
     +
      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);
     +
     +      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.

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

[Ville Voutilainen]

On Fri, 6 Nov 2020 at 23:14, Paul Scharnofske via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>      +          // 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();
>      +            }
>      +        }

Just calling join() will have the same effect, without having to
bother with the additional catching.

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

[Paul Scharnofske]

You are correct, I was not aware of this 
(https://stackoverflow.com/a/36393265/8746648).

Here is the simplified patch (mirror: 
https://static.asynts.com/2020/11/06/jthread-0002.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.
+
  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..a5d60fe7f9f 100644
--- a/libstdc++-v3/include/std/thread
+++ b/libstdc++-v3/include/std/thread
@@ -456,7 +456,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
      operator=(const jthread&) = delete;

      jthread&
-    operator=(jthread&&) noexcept = default;
+    operator=(jthread&& __other) noexcept
+    {
+      if (joinable())
+        {
+          request_stop();
+          join();
+        }
+
+      swap(__other);
+
+      return *this;
+    }

      void
      swap(jthread& __other) noexcept

On 11/6/20 10:35 PM, Ville Voutilainen wrote:
> On Fri, 6 Nov 2020 at 23:14, Paul Scharnofske via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>>       +          // 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();
>>       +            }
>>       +        }
> Just calling join() will have the same effect, without having to
> bother with the additional catching.

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

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

[Paul Scharnofske]

>I think this would work:
> 
>   jthread& operator=(jthread&& __x) noexcept
>   {
>     std::jthread(std::move(__x)).swap(*this);
>     return *this;
>   }

That looks a lot better than what I did, it's also consistent with other
places like std::stop_token::operator=(std::stop_token&&).

I updated my patch and also created a test that checks the post
conditions described in the standard.

---
 libstdc++-v3/include/std/thread               |  6 +++++-
 .../testsuite/30_threads/jthread/jthread.cc   | 20 +++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/std/thread b/libstdc++-v3/include/std/thread
index 887ee579962..080036e2609 100644
--- a/libstdc++-v3/include/std/thread
+++ b/libstdc++-v3/include/std/thread
@@ -456,7 +456,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     operator=(const jthread&) = delete;
 
     jthread&
-    operator=(jthread&&) noexcept = default;
+    operator=(jthread&& __other) noexcept
+    {
+      std::jthread(std::move(__other)).swap(*this);
+      return *this;
+    }
 
     void
     swap(jthread& __other) noexcept
diff --git a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
index 746ff437c1d..b8ba62f6df2 100644
--- a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
+++ b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
@@ -187,6 +187,25 @@ void test_detach()
   VERIFY(t1FinallyInterrupted.load());
 }
 
+//------------------------------------------------------
+
+void test_move_assignment()
+{
+    std::jthread thread1([]{});
+    std::jthread thread2([]{});
+
+    const auto id2 = thread2.get_id();
+    const auto ssource2 = thread2.get_stop_source();
+
+    thread1 = std::move(thread2);
+
+    VERIFY(thread1.get_id() == id2);
+    VERIFY(thread2.get_id() == std::jthread::id());
+
+    VERIFY(thread1.get_stop_source() == ssource2);
+    VERIFY(!thread2.get_stop_source().stop_possible());
+}
+
 int main()
 {
   std::set_terminate([](){
@@ -197,4 +216,5 @@ int main()
   test_stop_token();
   test_join();
   test_detach();
+  test_move_assignment();
 }
-- 
2.29.2

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

[Jonathan Wakely]

On 08/11/20 14:51 +0100, Paul Scharnofske via Libstdc++ wrote:
>>I think this would work:
>>
>>   jthread& operator=(jthread&& __x) noexcept
>>   {
>>     std::jthread(std::move(__x)).swap(*this);
>>     return *this;
>>   }
>
>That looks a lot better than what I did, it's also consistent with other
>places like std::stop_token::operator=(std::stop_token&&).
>
>I updated my patch and also created a test that checks the post
>conditions described in the standard.

Thanks, I've committed the patch and will also backport it to the
gcc-10 branch.

This patch is small enough to not require a copyright assignment, but
if you would like to make further contritbutions please contact me
off-list to discuss the legal prerequisites as described at
https://gcc.gnu.org/contribute.html

Thanks again for contributing to GCC!


>---
> libstdc++-v3/include/std/thread               |  6 +++++-
> .../testsuite/30_threads/jthread/jthread.cc   | 20 +++++++++++++++++++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/include/std/thread b/libstdc++-v3/include/std/thread
>index 887ee579962..080036e2609 100644
>--- a/libstdc++-v3/include/std/thread
>+++ b/libstdc++-v3/include/std/thread
>@@ -456,7 +456,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     operator=(const jthread&) = delete;
>
>     jthread&
>-    operator=(jthread&&) noexcept = default;
>+    operator=(jthread&& __other) noexcept
>+    {
>+      std::jthread(std::move(__other)).swap(*this);
>+      return *this;
>+    }
>
>     void
>     swap(jthread& __other) noexcept
>diff --git a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
>index 746ff437c1d..b8ba62f6df2 100644
>--- a/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
>+++ b/libstdc++-v3/testsuite/30_threads/jthread/jthread.cc
>@@ -187,6 +187,25 @@ void test_detach()
>   VERIFY(t1FinallyInterrupted.load());
> }
>
>+//------------------------------------------------------
>+
>+void test_move_assignment()
>+{
>+    std::jthread thread1([]{});
>+    std::jthread thread2([]{});
>+
>+    const auto id2 = thread2.get_id();
>+    const auto ssource2 = thread2.get_stop_source();
>+
>+    thread1 = std::move(thread2);
>+
>+    VERIFY(thread1.get_id() == id2);
>+    VERIFY(thread2.get_id() == std::jthread::id());
>+
>+    VERIFY(thread1.get_stop_source() == ssource2);
>+    VERIFY(!thread2.get_stop_source().stop_possible());
>+}
>+
> int main()
> {
>   std::set_terminate([](){
>@@ -197,4 +216,5 @@ int main()
>   test_stop_token();
>   test_join();
>   test_detach();
>+  test_move_assignment();
> }
>-- 
>2.29.2
>