* std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
@ 2020-11-06 21:13 Paul Scharnofske
2020-11-06 21:35 ` Ville Voutilainen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paul Scharnofske @ 2020-11-06 21:13 UTC (permalink / raw)
To: libstdc++
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
2020-11-06 21:13 std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread Paul Scharnofske
@ 2020-11-06 21:35 ` Ville Voutilainen
2020-11-06 21:49 ` Paul Scharnofske
2020-11-06 22:02 ` Jonathan Wakely
2020-11-08 13:51 ` Paul Scharnofske
2 siblings, 1 reply; 6+ messages in thread
From: Ville Voutilainen @ 2020-11-06 21:35 UTC (permalink / raw)
To: Paul Scharnofske; +Cc: libstdc++
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
2020-11-06 21:35 ` Ville Voutilainen
@ 2020-11-06 21:49 ` Paul Scharnofske
0 siblings, 0 replies; 6+ messages in thread
From: Paul Scharnofske @ 2020-11-06 21:49 UTC (permalink / raw)
To: Ville Voutilainen; +Cc: libstdc++
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
2020-11-06 21:13 std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread Paul Scharnofske
2020-11-06 21:35 ` Ville Voutilainen
@ 2020-11-06 22:02 ` Jonathan Wakely
2020-11-08 13:51 ` Paul Scharnofske
2 siblings, 0 replies; 6+ messages in thread
From: Jonathan Wakely @ 2020-11-06 22:02 UTC (permalink / raw)
To: Paul Scharnofske; +Cc: libstdc++
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
2020-11-06 21:13 std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread Paul Scharnofske
2020-11-06 21:35 ` Ville Voutilainen
2020-11-06 22:02 ` Jonathan Wakely
@ 2020-11-08 13:51 ` Paul Scharnofske
2020-11-11 11:15 ` Jonathan Wakely
2 siblings, 1 reply; 6+ messages in thread
From: Paul Scharnofske @ 2020-11-08 13:51 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++
>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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread.
2020-11-08 13:51 ` Paul Scharnofske
@ 2020-11-11 11:15 ` Jonathan Wakely
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Wakely @ 2020-11-11 11:15 UTC (permalink / raw)
To: Paul Scharnofske; +Cc: libstdc++, gcc-patches
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-11 11:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-06 21:13 std::jthread::operator=(std::jthread&&) calls std::terminate if *this has an associated running thread Paul Scharnofske
2020-11-06 21:35 ` Ville Voutilainen
2020-11-06 21:49 ` Paul Scharnofske
2020-11-06 22:02 ` Jonathan Wakely
2020-11-08 13:51 ` Paul Scharnofske
2020-11-11 11:15 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).