Skip to content

Commit 9c5fc40

Browse files
tomokinathiroyuki-komatsu
authored andcommitted
Make BackgroundFuture properly movable.
Currently moved out `BackgroundFuture` holds an empty thread handle, which is not joinable, and destructor crashes. PiperOrigin-RevId: 621071166
1 parent d7b9968 commit 9c5fc40

File tree

2 files changed

+33
-5
lines changed

2 files changed

+33
-5
lines changed

src/base/thread.h

+13-5
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,9 @@ namespace mozc {
4747
// Represents a thread, exposing a subset of `std::thread` APIs.
4848
//
4949
// Most notably, threads are undetachable unlike `std::thread`, thus must be
50-
// `join()`ed before destruction. This means that the `mozc::Thread` instance
51-
// must be retained even for a long-running one, though which may be until
52-
// the end of the process.
50+
// `Join()`ed before destruction if `Joinable()`. This means that the
51+
// `mozc::Thread` instance must be retained even for a long-running one, though
52+
// which may be until the end of the process.
5353
//
5454
// The semantics of the present APIs are mostly the same as `std::thread`
5555
// counterpart of the same (but lowercase) name, except that the behavior of
@@ -77,6 +77,8 @@ class Thread {
7777
Thread(Thread &&) noexcept = default;
7878
Thread &operator=(Thread &&) noexcept = default;
7979

80+
bool Joinable() const noexcept { return thread_.joinable(); }
81+
8082
void Join() { thread_.join(); }
8183

8284
private:
@@ -182,7 +184,9 @@ BackgroundFuture<R>::BackgroundFuture(F &&f, Args &&...args)
182184

183185
template <class R>
184186
BackgroundFuture<R>::~BackgroundFuture() {
185-
thread_.Join();
187+
if (thread_.Joinable()) {
188+
thread_.Join();
189+
}
186190
}
187191

188192
template <class R>
@@ -229,7 +233,11 @@ BackgroundFuture<void>::BackgroundFuture(F &&f, Args &&...args)
229233
done.Notify();
230234
}) {}
231235

232-
inline BackgroundFuture<void>::~BackgroundFuture() { thread_.Join(); }
236+
inline BackgroundFuture<void>::~BackgroundFuture() {
237+
if (thread_.Joinable()) {
238+
thread_.Join();
239+
}
240+
}
233241

234242
inline void BackgroundFuture<void>::Wait() const {
235243
done_->WaitForNotification();

src/base/thread_test.cc

+20
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
#include <atomic>
3333
#include <memory>
34+
#include <optional>
3435
#include <utility>
3536

3637
#include "absl/synchronization/notification.h"
@@ -93,6 +94,16 @@ TEST(ThreadTest, CopiesThingsAtMostOnce) {
9394
EXPECT_EQ(c2->load(), 0);
9495
}
9596

97+
TEST(ThreadTest, Joinable) {
98+
Thread default_constructed;
99+
EXPECT_FALSE(default_constructed.Joinable());
100+
101+
Thread real_work([] {});
102+
EXPECT_TRUE(real_work.Joinable());
103+
real_work.Join();
104+
EXPECT_FALSE(real_work.Joinable());
105+
}
106+
96107
TEST(BackgroundFutureTest, ReturnsComputedValueOnReady) {
97108
auto future = BackgroundFuture<int>([] {
98109
absl::SleepFor(absl::Milliseconds(100));
@@ -164,5 +175,14 @@ TEST(BackgroundFutureTest, CopiesThingsAtMostOnce) {
164175
}
165176
}
166177

178+
TEST(BackgroundFutureTest, DestructingMovedOutFutureDoesNotCrash) {
179+
std::optional<BackgroundFuture<int>> f;
180+
{
181+
auto g = BackgroundFuture<int>([] { return 42; });
182+
f = std::move(g);
183+
}
184+
EXPECT_EQ(f->Get(), 42);
185+
}
186+
167187
} // namespace
168188
} // namespace mozc

0 commit comments

Comments
 (0)