[libc++] Don't try to wait on a thread that hasn't started in std::async, take 2 #130145
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kadircet
reviewed
Mar 7, 2025
|
also forgot to mention, but yes, the bits that actually revert changes in https://github.com/llvm/llvm-project/pull/125433/files fix issues we're seeing (and test case here already demonstrates the situation). |
philnik777
force-pushed
the
fix_future_wait
branch
from
5397c2e to
8e7f5c5
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
philnik777
force-pushed
the
fix_future_wait
branch
from
8e7f5c5 to
d25ee68
Compare
philnik777
changed the title
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesIf the creation of a thread fails, this causes an idle loop that will never end because the thread wasn't started in the first place. This also adds a test for the regression reported in #125433 to make sure we're not reintroducing it later. Fixes #125428 Full diff: https://github.com/llvm/llvm-project/pull/130145.diff 3 Files Affected:
diff --git a/libcxx/include/future b/libcxx/include/future
index 3dfcce80a977d..d791a6f3a5581 100644
--- a/libcxx/include/future
+++ b/libcxx/include/future
@@ -1823,7 +1823,16 @@ template <class _Rp, class _Fp>
_LIBCPP_HIDE_FROM_ABI future<_Rp> __make_async_assoc_state(_Fp&& __f) {
unique_ptr<__async_assoc_state<_Rp, _Fp>, __release_shared_count> __h(
new __async_assoc_state<_Rp, _Fp>(std::forward<_Fp>(__f)));
- std::thread(&__async_assoc_state<_Rp, _Fp>::__execute, __h.get()).detach();
+#if _LIBCPP_HAS_EXCEPTIONS
+ try {
+#endif
+ std::thread(&__async_assoc_state<_Rp, _Fp>::__execute, __h.get()).detach();
+#if _LIBCPP_HAS_EXCEPTIONS
+ } catch (...) {
+ __h->__make_ready();
+ throw;
+ }
+#endif
return future<_Rp>(__h.get());
}
diff --git a/libcxx/test/std/thread/futures/futures.async/thread_create_failure.pass.cpp b/libcxx/test/std/thread/futures/futures.async/thread_create_failure.pass.cpp
new file mode 100644
index 0000000000000..9ab8296d49af1
--- /dev/null
+++ b/libcxx/test/std/thread/futures/futures.async/thread_create_failure.pass.cpp
@@ -0,0 +1,60 @@
+//===----------------------------------------------------------------------===//
+//
+// 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, no-exceptions
+
+// ASan seems to try to create threadsm which obviouly doesn't work in this test.
+// UNSUPPORTED: asan
+
+// UNSUPPORTED: c++03
+
+// There is no way to limit the number of threads on windows
+// UNSUPPORTED: windows
+
+// AIX and macOS seem to limit the number of processes, not threads via RLIMIT_NPROC
+// XFAIL: target={{.+}}-aix{{.*}}
+// XFAIL: target={{.+}}-apple-{{.*}}
+
+// This test makes sure that we fail gracefully in care the thread creation fails. This is only reliably possible on
+// systems that allow limiting the number of threads that can be created. See https://llvm.org/PR125428 for more details
+
+#include <cassert>
+#include <future>
+#include <system_error>
+
+#if __has_include(<sys/resource.h>)
+# include <sys/resource.h>
+# ifdef RLIMIT_NPROC
+void force_thread_creation_failure() {
+ rlimit lim = {1, 1};
+ assert(setrlimit(RLIMIT_NPROC, &lim) == 0);
+}
+# else
+# error "No known way to force only one thread being available"
+# endif
+#else
+# error "No known way to force only one thread being available"
+#endif
+
+int main(int, char**) {
+ force_thread_creation_failure();
+
+ try {
+ std::future<int> fut = std::async(std::launch::async, [] { return 1; });
+ assert(false);
+ } catch (const std::system_error&) {
+ }
+
+ try {
+ std::future<void> fut = std::async(std::launch::async, [] { return; });
+ assert(false);
+ } catch (const std::system_error&) {
+ }
+
+ return 0;
+}
diff --git a/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
new file mode 100644
index 0000000000000..ff771a68c6bef
--- /dev/null
+++ b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
@@ -0,0 +1,37 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+// UNSUPPORTED: c++03
+
+// This test uses std::atomic interfaces that are only available in C++20
+// UNSUPPORTED: c++11, c++14, c++17
+
+// Make sure that the `future` destructor keeps the data alive until the thread finished. This test fails by triggering
+// TSan. It may not be observable by normal means.
+
+#include <atomic>
+#include <future>
+#include <mutex>
+
+std::mutex mux;
+
+int main() {
+ using namespace std::chrono_literals;
+ std::unique_lock lock(mux);
+ std::atomic<bool> in_async = false;
+ auto v = std::async(std::launch::async, [&in_async, value = 1]() mutable {
+ in_async = true;
+ in_async.notify_all();
+ std::scoped_lock thread_lock(mux);
+ value = 4;
+ (void)value;
+ });
+ in_async.wait(true);
+ lock.unlock();
+}
|
philnik777
force-pushed
the
fix_future_wait
branch
from
d25ee68 to
79c001f
Compare
philnik777
added a commit
that referenced
this pull request
Jun 26, 2025
I've accidentally merged the PR before addressing all comments. This patch fixes the remaining ones.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/13334 Here is the relevant piece of the build log for the reference |
|
Hi, this PR broken the sanitizer buildbot. Can you take a look? |
This looks quite similar to ASan. I guess HWAsan also creates threads? In that case we can probably mark the test as unsupported for HWAsan. Also, please note that HWAsan with libc++ is currently not a supported configuration, since we don't have precommit CI. Please work with us so we can get coverage with HWAsan in the precommit CI so that these kinds of problems don't get merged into main at all. |
qinkunbao
added a commit
that referenced
this pull request
Jun 26, 2025
Done. Thanks for the comments.
We don't have a precommit CI for Sanitizers as well. There are some bots that build the repo periodically. Rumors say running the sanitizer bots as-is already costs a 6-figure sum per year. |
qinkunbao
added a commit
that referenced
this pull request
Jun 26, 2025
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
…ync, take 2 (llvm#130145) If the creation of a thread fails, this causes an idle loop that will never end because the thread wasn't started in the first place. This also adds a test for the regression reported in llvm#125433 to make sure we're not reintroducing it later. Fixes llvm#125428
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
I've accidentally merged the PR before addressing all comments. This patch fixes the remaining ones.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
It's not about that, our, sanitizer bots are heavy as we do 3-stage builds with sanitizers. Here we just need to add HWAsan to already existing libc++ CI. @philnik777 Please comment #145970 if you have a particular pointers where to start. |
|
I see time outs for the added testcase wait_on_destruct.pass.cpp every now and then. |
|
@mikaelholmen see #146240 |
Thanks! |
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
…ync, take 2 (llvm#130145) If the creation of a thread fails, this causes an idle loop that will never end because the thread wasn't started in the first place. This also adds a test for the regression reported in llvm#125433 to make sure we're not reintroducing it later. Fixes llvm#125428
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
I've accidentally merged the PR before addressing all comments. This patch fixes the remaining ones.
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the creation of a thread fails, this causes an idle loop that will never end because the thread wasn't started in the first place.
This also adds a test for the regression reported in #125433 to make sure we're not reintroducing it later.
Fixes #125428