Skip to content

Commit 769e1df

Browse files
committed
Firestore: Fix memory leak in thread_safe_memoizer.h
1 parent f53dfc3 commit 769e1df

File tree

8 files changed

+56
-61
lines changed

8 files changed

+56
-61
lines changed

Firestore/core/src/core/composite_filter.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,12 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter(
143143

144144
const std::vector<FieldFilter>& CompositeFilter::Rep::GetFlattenedFilters()
145145
const {
146-
return memoized_flattened_filters_->memoize([&]() {
147-
std::vector<FieldFilter> flattened_filters;
146+
return memoized_flattened_filters_.memoize([&]() {
147+
auto flattened_filters = std::make_unique<std::vector<FieldFilter>>();
148148
for (const auto& filter : filters())
149149
std::copy(filter.GetFlattenedFilters().begin(),
150150
filter.GetFlattenedFilters().end(),
151-
std::back_inserter(flattened_filters));
151+
std::back_inserter(*flattened_filters));
152152
return flattened_filters;
153153
});
154154
}

Firestore/core/src/core/field_filter.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,10 @@ FieldFilter::FieldFilter(std::shared_ptr<const Filter::Rep> rep)
124124

125125
const std::vector<FieldFilter>& FieldFilter::Rep::GetFlattenedFilters() const {
126126
// This is already a field filter, so we return a vector of size one.
127-
return memoized_flattened_filters_->memoize([&]() {
128-
return std::vector<FieldFilter>{
129-
FieldFilter(std::make_shared<const Rep>(*this))};
127+
return memoized_flattened_filters_.memoize([&]() {
128+
auto filters = std::make_unique<std::vector<FieldFilter>>();
129+
filters->push_back(FieldFilter(std::make_shared<const Rep>(*this)));
130+
return filters;
130131
});
131132
}
132133

Firestore/core/src/core/filter.cc

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,6 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) {
3535
return os << filter.ToString();
3636
}
3737

38-
Filter::Rep::Rep()
39-
: memoized_flattened_filters_(
40-
std::make_shared<
41-
util::ThreadSafeMemoizer<std::vector<FieldFilter>>>()) {
42-
}
43-
4438
} // namespace core
4539
} // namespace firestore
4640
} // namespace firebase

Firestore/core/src/core/filter.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,6 @@ class Filter {
114114
protected:
115115
class Rep {
116116
public:
117-
Rep();
118-
119117
virtual ~Rep() = default;
120118

121119
virtual Type type() const {
@@ -160,7 +158,7 @@ class Filter {
160158
* (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag`
161159
* member variable, which is not copyable).
162160
*/
163-
mutable std::shared_ptr<util::ThreadSafeMemoizer<std::vector<FieldFilter>>>
161+
mutable util::ThreadSafeMemoizer<std::vector<FieldFilter>>
164162
memoized_flattened_filters_;
165163
};
166164

Firestore/core/src/core/query.cc

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,9 @@ absl::optional<Operator> Query::FindOpInsideFilters(
9292
}
9393

9494
const std::vector<OrderBy>& Query::normalized_order_bys() const {
95-
return memoized_normalized_order_bys_->memoize([&]() {
95+
return memoized_normalized_order_bys_.memoize([&]() {
9696
// Any explicit order by fields should be added as is.
97-
std::vector<OrderBy> result = explicit_order_bys_;
97+
auto result = std::make_unique<std::vector<OrderBy>>(explicit_order_bys_);
9898
std::set<FieldPath> fieldsNormalized;
9999
for (const OrderBy& order_by : explicit_order_bys_) {
100100
fieldsNormalized.insert(order_by.field());
@@ -117,14 +117,14 @@ const std::vector<OrderBy>& Query::normalized_order_bys() const {
117117
for (const model::FieldPath& field : inequality_fields) {
118118
if (fieldsNormalized.find(field) == fieldsNormalized.end() &&
119119
!field.IsKeyFieldPath()) {
120-
result.push_back(OrderBy(field, last_direction));
120+
result->push_back(OrderBy(field, last_direction));
121121
}
122122
}
123123

124124
// Add the document key field to the last if it is not explicitly ordered.
125125
if (fieldsNormalized.find(FieldPath::KeyFieldPath()) ==
126126
fieldsNormalized.end()) {
127-
result.push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction));
127+
result->push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction));
128128
}
129129

130130
return result;
@@ -297,13 +297,15 @@ std::string Query::ToString() const {
297297
}
298298

299299
const Target& Query::ToTarget() const& {
300-
return memoized_target_->memoize(
301-
[&]() { return ToTarget(normalized_order_bys()); });
300+
return memoized_target_.memoize([&]() {
301+
return std::make_unique<Target>(ToTarget(normalized_order_bys()));
302+
});
302303
}
303304

304305
const Target& Query::ToAggregateTarget() const& {
305-
return memoized_aggregate_target_->memoize(
306-
[&]() { return ToTarget(explicit_order_bys_); });
306+
return memoized_aggregate_target_.memoize([&]() {
307+
return std::make_unique<Target>(ToTarget(explicit_order_bys_));
308+
});
307309
}
308310

309311
Target Query::ToTarget(const std::vector<OrderBy>& order_bys) const {

Firestore/core/src/core/query.h

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -295,20 +295,16 @@ class Query {
295295
// member variable, which is not copyable).
296296

297297
// The memoized list of sort orders.
298-
mutable std::shared_ptr<util::ThreadSafeMemoizer<std::vector<OrderBy>>>
299-
memoized_normalized_order_bys_{
300-
std::make_shared<util::ThreadSafeMemoizer<std::vector<OrderBy>>>()};
298+
mutable util::ThreadSafeMemoizer<std::vector<OrderBy>>
299+
memoized_normalized_order_bys_;
301300

302301
// The corresponding Target of this Query instance.
303-
mutable std::shared_ptr<util::ThreadSafeMemoizer<Target>> memoized_target_{
304-
std::make_shared<util::ThreadSafeMemoizer<Target>>()};
302+
mutable util::ThreadSafeMemoizer<Target> memoized_target_;
305303

306304
// The corresponding aggregate Target of this Query instance. Unlike targets
307305
// for non-aggregate queries, aggregate query targets do not contain
308306
// normalized order-bys, they only contain explicit order-bys.
309-
mutable std::shared_ptr<util::ThreadSafeMemoizer<Target>>
310-
memoized_aggregate_target_{
311-
std::make_shared<util::ThreadSafeMemoizer<Target>>()};
307+
mutable util::ThreadSafeMemoizer<Target> memoized_aggregate_target_;
312308
};
313309

314310
bool operator==(const Query& lhs, const Query& rhs);

Firestore/core/src/util/thread_safe_memoizer.h

Lines changed: 32 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,7 @@
1818
#define FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_
1919

2020
#include <functional>
21-
#include <mutex> // NOLINT(build/c++11)
22-
#include <vector>
21+
#include <memory>
2322

2423
namespace firebase {
2524
namespace firestore {
@@ -36,43 +35,47 @@ class ThreadSafeMemoizer {
3635
public:
3736
ThreadSafeMemoizer() = default;
3837

39-
~ThreadSafeMemoizer() {
40-
// Call `std::call_once` in order to synchronize with the "active"
41-
// invocation of `memoize()`. Without this synchronization, there is a data
42-
// race between this destructor, which "reads" `memoized_value_` to destroy
43-
// it, and the write to `memoized_value_` done by the "active" invocation of
44-
// `memoize()`.
45-
std::call_once(once_, [&]() {});
38+
ThreadSafeMemoizer(const ThreadSafeMemoizer&) {
39+
}
40+
41+
ThreadSafeMemoizer& operator=(const ThreadSafeMemoizer&) {
42+
return *this;
4643
}
4744

48-
// This class cannot be copied or moved, because it has `std::once_flag`
49-
// member.
50-
ThreadSafeMemoizer(const ThreadSafeMemoizer&) = delete;
51-
ThreadSafeMemoizer(ThreadSafeMemoizer&&) = delete;
52-
ThreadSafeMemoizer& operator=(const ThreadSafeMemoizer&) = delete;
53-
ThreadSafeMemoizer& operator=(ThreadSafeMemoizer&&) = delete;
45+
ThreadSafeMemoizer(ThreadSafeMemoizer&& other) = default;
46+
ThreadSafeMemoizer& operator=(ThreadSafeMemoizer&& other) = default;
47+
48+
~ThreadSafeMemoizer() {
49+
delete memoized_.load();
50+
}
5451

5552
/**
5653
* Memoize a value.
5754
*
58-
* The std::function object specified by the first invocation of this
59-
* function (the "active" invocation) will be invoked synchronously.
60-
* None of the std::function objects specified by the subsequent
61-
* invocations of this function (the "passive" invocations) will be
62-
* invoked. All invocations, both "active" and "passive", will return a
63-
* reference to the std::vector created by copying the return value from
64-
* the std::function specified by the "active" invocation. It is,
65-
* therefore, the "active" invocation's job to return the std::vector
66-
* to memoize.
55+
* If there is no memoized value then the given function is called to create
56+
* the value, returning a reference to the created value and storing the
57+
* pointer to the created value. On the other hand, if there _is_ a memoized
58+
* value from a previous invocation then a reference to that object is
59+
* returned and the given function is not called. Note that the given function
60+
* may be called more than once and, therefore, must be idempotent.
6761
*/
68-
const T& memoize(std::function<T()> func) {
69-
std::call_once(once_, [&]() { memoized_value_ = func(); });
70-
return memoized_value_;
62+
const T& memoize(std::function<std::unique_ptr<T>()> func) {
63+
while (true) {
64+
T* old_memoized = memoized_.load();
65+
if (old_memoized) {
66+
return *old_memoized;
67+
}
68+
69+
std::unique_ptr<T> new_memoized = func();
70+
71+
if (memoized_.compare_exchange_strong(old_memoized, new_memoized.get())) {
72+
return *new_memoized.release();
73+
}
74+
}
7175
}
7276

7377
private:
74-
std::once_flag once_;
75-
T memoized_value_;
78+
std::atomic<T*> memoized_ = {nullptr};
7679
};
7780

7881
} // namespace util

Firestore/core/test/unit/util/thread_safe_memoizer_test.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "Firestore/core/src/util/thread_safe_memoizer.h"
1818

19+
#include <memory> // NOLINT(build/c++11)
1920
#include <thread> // NOLINT(build/c++11)
2021
#include "gtest/gtest.h"
2122

@@ -32,7 +33,7 @@ TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) {
3233
// If the lambda gets executed multiple times, threads will see incremented
3334
// `global_int`.
3435
global_int++;
35-
return global_int.load();
36+
return std::make_unique<int>(global_int.load());
3637
};
3738

3839
const int num_threads = 5;

0 commit comments

Comments
 (0)