Hold mutex lock while notify_all is called at notify_all_at_thread_exit

[ldionne]

Summary

Releasing the mutex before the call to notify_all is an optimization.
This optimization cannot be used here. The thread waiting on the
condition might destroy the associated resources — mutex + condition
variable — and the notifier thread will access an destroyed variable
— the condition variable. In fact, notify_all_at_thread_exit is meant
exactly to join on detached threads, and the waiting thread doesn't
expect for the notifier thread to access any further shared resources,
making this scenario very likely to happen. The waiting thread might
awake spuriously on the release of the mutex lock. The reorder is
necessary to prevent this race.

Further details can be found at
https://cplusplus.github.io/LWG/issue3343.

[• Quuxplusone]

LGTM based on a quick read of that LWG issue. @ldionne, WDYT?

[xgupta]

Kadir, ig it is enough for today 😂

[MaskRay]

I disabled post.kadirselcuk who is apparently spamming the website.

[ldionne]

The change does look legit, but the submitter not so much. We should:

1. Add a test
2. Update the LWG status page

Edit Oh, sorry, I thought the submitter was the spammer, not @vinipsmaker. In all cases, my comments above hold but let's move forward with this.

[vinipsmaker]

Update the LWG status page

Given you'll spend time with the committee anyway, could one of you also write a related unlock_at_thread_exit? The use case is described at https://lists.isocpp.org/std-proposals/2021/07/2809.php.

[• Quuxplusone]

Commandeering to add a test and update the LWG status page, per ldionne's request.

[vinipsmaker]

Commandeering to add a test and update the LWG status page, per ldionne's request.

What exactly "commandeering" refers to here? I don't understand this comment.

[• Quuxplusone]

@vinipsmaker wrote:

What exactly "commandeering" refers to here?

It means I'm taking over the responsibility of updating this patch, i.e., it's "mine" now instead of "yours" (as far as Phabricator is concerned). Louis's impression was that you weren't planning to follow up on adding tests and updating the status page, so he asked me to commandeer the patch. However, if you want it back so that you can add tests and status-page update, then I have no problem with you commandeering it back. I'm not planning to actively work on it for a few hours at least. :)

Btw, "could one of you also write a related unlock_at_thread_exit?" — I agree with your rationale and think such a function ought to exist, but I don't think @ldionne would consent to add it to libc++ unless it was already in the Standard (or at least tentatively ready for adoption by the Standard), and I'm not personally willing to write the paper and champion it and so on. But I think it's a good idea for someone (not me) to write that paper. :)

[vinipsmaker]

It means I'm taking over the responsibility of updating this patch, i.e., it's "mine" now instead of "yours" (as far as Phabricator is concerned).

Okay, I see. Thanks.

Louis's impression was that you weren't planning to follow up on adding tests and updating the status page, so he asked me to commandeer the patch.

His impression is correct. I just want to solve my problem. I want to stay as far away as possible from C++ committees. Thanks for taking care of this.

[...] I agree with your rationale and think such a function [unlock_at_thread_exit] ought to exist, but I don't think @ldionne would consent to add it to libc++ unless it was already in the Standard [...] But I think it's a good idea for someone (not me) to write that paper. :)

I didn't express myself clearly. Actually that's what I was suggesting... a paper. Several of you guys already are in constant contact with the C++ committee. I just want to solve my problem. I don't want my name in the standard nor any contact with the C++ committee. Just like yourself... I don't want to write such paper.

Looks like this function won't see the light of the day, but that's okay.

[• Quuxplusone]

libcxx/src/thread.cpp

162

Btw, update: I've emailed @lewissbaker to ask why the proof-of-concept wandbox from https://cplusplus.github.io/LWG/issue3343 (namely, https://wandbox.org/permlink/eUu3eiQbLl7JQKMm ) doesn't reproduce anymore. It's trivial to turn that code snippet into a libc++ unit test; but since it's already green on trunk, it wouldn't be very useful.
He hasn't gotten back to me yet. But this PR is still on my radar!

[• Quuxplusone]

Still haven't heard from @lewissbaker , so this did fall off my radar. At this point I'm inclined to say we should just commit the fix, presumably without the attached test case (because the test case doesn't actually reproduce the bug with libc++ anyway). However, I'm showing the test case in this PR right now, just so that people see what I tried.
Again, this test case does not reproduce the bug: it's green both before and after the proposed change.

[vinipsmaker]

At this point I'm inclined to say we should just commit the fix, presumably without the attached test case

Well... I can't even use notify_all_at_thread_exit in its current state, and my workaround isn't general enough to cover all cases. Tests should be available to ensure the library is correct, not the other way around. If a test case is just going to guarantee libc++ will be buggy... I don't see the point with a test case at all.

[• Quuxplusone]

@vinipsmaker wrote:

If a test case is just going to guarantee libc++ will be buggy...

Well, this test case is green both before and after the change. So it wouldn't "guarantee libc++ will be buggy"; it simply would "not guarantee libc++ won't be buggy."

And actually it looks like this test case does fail with GCC 11! https://reviews.llvm.org/harbormaster/unit/view/943944/
Is that bot using libstdc++ instead of libc++? or is something weird going on here?

[• Quuxplusone]

@ldionne, what are your thoughts?
Situation at the moment: This apparently fixes a fairly important bug in <thread>. The attached test case is supposed to detect the bug, but AFAICT it does not. The wording for the fix is proposed in LWG3343 but has not yet been adopted into any draft.
I think the obvious alternatives are "Land this to fix the bug and get it over with," or "Continue sitting on this indefinitely until LWG3343 gets adopted." Either way, we'll have to grapple with the fact that the test case doesn't seem to reproduce the bug.

(Also, re-ping: @lewissbaker , but I don't think he's listening)

[ldionne]

LGTM, but..

Is that bot using libstdc++ instead of libc++? or is something weird going on here?

No, it should be using libc++ (which you can validate by looking at the command-line used to compile the test). Are you able to reproduce in Docker with this?

./libcxx/utils/ci/run-buildbot-container
$ ./libcxx/utils/ci/run-buildbot generic-gcc-cxx11

[lewissbaker]

libcxx/src/thread.cpp

162

One approach you could take to try to verify that this fix works is to try putting a sleep between the unlock() and notify_all() calls in both the before and after code to try to increase the chances of the other thread acquires the lock, sees the state that says the thread is done and then destroys the CV before the exiting thread calls notify_all().

It's obviously not something you'd leave in the code, however, but could serve to give a higher confidence in the fix.

libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp

63

We are trying to make the cv destructor race with the call to notify_all().
This is generally only going to happen if the main thread happens to see the threads_active as 0 between the lock being released and the cv being notified.

You could possibly increase the likelihood of a race by taking out a lock on the mutex before launching the threads.
The idea being that the other threads that launch quickly will wait to acquire the lock and then once the main thread releases the lock inside the call to cv.wait() then the other threads start at once.

I'd also consider removing the sleep_for() in the threads to encourage some more raciness of all of the threads exiting at the same time.
Really, it's only ever going to be the last thread to execute that will potentially run into the lifetime issue.

Another approach could be to have the main thread simply spin, acquiring the lock, looking at the threads_active and then releasing the lock and having it continue on and destroy the CV once it sees that no more threads are active.
i.e. just do a spin-loop version of cv.wait() without actually calling cv.wait()

If you do all of this in conjunction with inserting a sleep_for() call in the implementation of notify_all_at_thread_exit() between the unlock and notify_all calls (just for testing) then you should be able to reproduce the issue.

[vinipsmaker]

Any news on the issue? More than one year later and I still can't use this fix. It's an one-line reordering guys, c'mon.

[ldionne]

This fell through the cracks. @vinipsmaker what Author Name <email@domain> do you want that committed under?

[ldionne]

Rebase onto main.

[ldionne]

Will ship once I have the attribution info and a green CI run.

[vinipsmaker]

This fell through the cracks. @vinipsmaker what Author Name <email@domain> do you want that committed under?

I only authored the diff for the file libcxx/src/thread.cpp and that's a 1-line patchset. This is so small that I don't believe it's even copyrightable. If you really need one, you can use this email for the author field: https://mailhide.io/en/e/ckagYWto ; and Vinícius Oliveira for the author name would be fine.

[ldionne]

In D105758#4041055, @vinipsmaker wrote:

This fell through the cracks. @vinipsmaker what Author Name <email@domain> do you want that committed under?

I only authored the diff for the file libcxx/src/thread.cpp and that's a 1-line patchset. This is so small that I don't believe it's even copyrightable. If you really need one, you can use this email for the author field: https://mailhide.io/en/e/ckagYWto ; and Vinícius Oliveira for the author name would be fine.

Oh, sorry, I see now that @Quuxplusone had authored most of the patch.

[ldionne]

Tweak test to avoid timeouts and fix for no-threads configuration.

[ldionne]

Fix LWG issue CSV

[ldionne]

Failures are flakes, landing.

libcxx/docs/Status/Cxx2bIssues.csv

 "`3818 <https://wg21.link/LWG3818>`__","Exposition-only concepts are not described in library intro", "November 2022","|Nothing to do|","",""
 "`3822 <https://wg21.link/LWG3822>`__","Avoiding normalization in ``filesystem::weakly_canonical``", "November 2022","","",""
 "`3823 <https://wg21.link/LWG3823>`__","Unnecessary precondition for ``is_aggregate``", "November 2022","","",""
 "`3824 <https://wg21.link/LWG3824>`__","Number of ``bind`` placeholders is underspecified", "November 2022","|Nothing to do|","",""
 "`3826 <https://wg21.link/LWG3826>`__","Redundant specification [for overload of yield_value]", "November 2022","","",""
 "","","","","",""
 "`3631 <https://wg21.link/LWG3631>`__","``basic_format_arg(T&&)`` should use ``remove_cvref_t<T>`` throughout","Not voted in","|Complete|","15.0",""
 "`3645 <https://wg21.link/LWG3645>`__","``resize_and_overwrite`` is overspecified to call its callback with lvalues","Not voted in","|Complete|","14.0",""
+"`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""

libcxx/src/thread.cpp

 public:
     __thread_struct_imp() {}
     ~__thread_struct_imp();
 
     void notify_all_at_thread_exit(condition_variable* cv, mutex* m);
     void __make_ready_at_thread_exit(__assoc_sub_state* __s);
 };
 
 __thread_struct_imp::~__thread_struct_imp()
 {
     for (_Notify::iterator i = notify_.begin(), e = notify_.end();
             i != e; ++i)
     {
-        i->second->unlock();
         i->first->notify_all();
+        i->second->unlock();
     }
     for (_AsyncStates::iterator i = async_states_.begin(), e = async_states_.end();
             i != e; ++i)
     {
         (*i)->__make_ready();
         (*i)->__release_shared();
     }
 }

libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: no-threads
 
-// notify_all_at_thread_exit(...) requires move semantics to transfer the
-// unique_lock.
+// notify_all_at_thread_exit(...) requires move semantics to transfer the unique_lock.
 // UNSUPPORTED: c++03
 
 // <condition_variable>
-
-// void
-//   notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
+//
+// void notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
 
 #include <condition_variable>
 #include <mutex>
 #include <thread>
 #include <chrono>
 #include <cassert>
 
 #include "make_test_thread.h"

libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: no-threads
+
+// notify_all_at_thread_exit(...) requires move semantics to transfer the unique_lock.
+// UNSUPPORTED: c++03
+
+// This is a regression test for LWG3343.
+//
+// <condition_variable>
+//
+// void notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
+
+#include "make_test_thread.h"
+#include "test_macros.h"
+
+#include <condition_variable>
+#include <cassert>
+#include <chrono>
+#include <memory>
+#include <mutex>
+#include <thread>
+
+union X {
+    X() : cv_() {}
+    ~X() {}
+    std::condition_variable cv_;
+    unsigned char bytes_[sizeof(std::condition_variable)];
+};
+
+void test()
+{
+    constexpr int N = 3;
+
+    X x;
+    std::mutex m;
+    int threads_active = N;
+
+    for (int i = 0; i < N; ++i) {
+        std::thread t = support::make_test_thread([&] {
+            // Signal thread completion
+            std::unique_lock<std::mutex> lk(m);
+            --threads_active;
+            std::notify_all_at_thread_exit(x.cv_, std::move(lk));
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+        });
+        t.detach();
+    }
+
+    // Wait until all threads complete, i.e. until they've all
+    // decremented `threads_active` and then unlocked `m` at thread exit.
+    // It is possible that this `wait` may spuriously wake up,
+    // but it won't be able to continue until the last thread
+    // unlocks `m`.
+    {
+        std::unique_lock<std::mutex> lk(m);
+        x.cv_.wait(lk, [&]() { return threads_active == 0; });
+    }
+
+    // Destroy the condition_variable and shred the bytes.
+    // Simulate reusing the memory for something else.
+    x.cv_.~condition_variable();
+    for (unsigned char& c : x.bytes_) {
+        c = 0xcd;
+    }
+
+    DoNotOptimize(x.bytes_);
+
+    // Check that the bytes still have the same value we just wrote to them.
+    // If any thread wrongly unlocked `m` before calling cv.notify_all(), and
+    // cv.notify_all() writes to the memory of the cv, then we have a chance
+    // to detect the problem here.
+    int sum = 0;
+    for (unsigned char c : x.bytes_) {
+       sum += c;
+    }
+    DoNotOptimize(sum);
+    assert(sum == (0xcd * sizeof(std::condition_variable)));
+}
+
+int main(int, char**)
+{
+    for (int i = 0; i < 1000; ++i) {
+        test();
+    }
+
+    return 0;
+}