8000 fix undefined behavior/race in query cache result moving (#6538) · mnemosdev/arangodb@02a08df · GitHub
[go: up one dir, main page]
Skip to content

Commit 02a08df

Browse files
jsteemannjsteemann
authored
fix undefined behavior/race in query cache result moving (arangodb#6538)
1 parent 5595317 commit 02a08df

File tree

3 files changed

+33
-110
lines changed

3 files changed

+33
-110
lines changed

‎arangod/Aql/Query.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,6 @@ QueryResult Query::execute(QueryRegistry* registry) {
538538
// check the query cache for an existing result
539539
auto cacheEntry = arangodb::aql::QueryCache::instance()->lookup(
540540
_vocbase, queryHash, _queryString);
541-
arangodb::aql::QueryCacheResultEntryGuard guard(cacheEntry);
542541

543542
if (cacheEntry != nullptr) {
544543
ExecContext const* exe = ExecContext::CURRENT;
@@ -728,10 +727,8 @@ QueryResultV8 Query::executeV8(v8::Isolate* isolate, QueryRegistry* registry) {
728727
// check the query cache for an existing result
729728
auto cacheEntry = arangodb::aql::QueryCache::instance()->lookup(
730729
_vocbase, queryHash, _queryString);
731-
arangodb::aql::QueryCacheResultEntryGuard guard(cacheEntry);
732730

733731
if (cacheEntry != nullptr) {
734-
735732
auto ctx = transaction::StandaloneContext::Create(_vocbase);
736733
ExecContext const* exe = ExecContext::CURRENT;
737734
// got a result from the query cache

‎arangod/Aql/QueryCache.cpp

Lines changed: 25 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ using namespace arangodb::aql;
3939
static arangodb::aql::QueryCache Instance;
4040

4141
/// @brief maximum number of results in each per-database cache
42-
static size_t MaxResults = 128; // default value. can be changed later
42+
static std::atomic<size_t> MaxResults(128); // default value. can be changed later
4343

4444
/// @brief whether or not the cache is enabled
4545
static std::atomic<arangodb::aql::QueryCacheMode> Mode(CACHE_ON_DEMAND);
@@ -58,30 +58,6 @@ QueryCacheResultEntry::QueryCacheResultEntry(
5858
_deletionRequested(0) {
5959
}
6060

61-
/// @brief check whether the element can be destroyed, and delete it if yes
62-
void QueryCacheResultEntry::tryDelete() {
63-
_deletionRequested = 1;
64-
65-
if (_refCount == 0) {
66-
delete this;
67-
}
68-
}
69-
70-
/// @brief use the element, so it cannot be deleted meanwhile
71-
void QueryCacheResultEntry::use() { ++_refCount; }
72-
73-
/// @brief unuse the element, so it can be deleted if required
74-
void QueryCacheResultEntry::unuse() {
75-
TRI_ASSERT(_refCount > 0);
76-
77-
if (--_refCount == 0) {
78-
if (_deletionRequested == 1) {
79-
// trigger the deletion
80-
delete this;
81-
}
82-
}
83-
}
84-
8561
/// @brief create a database-specific cache
8662
QueryCacheDatabaseEntry::QueryCacheDatabaseEntry()
8763
: _entriesByHash(),
@@ -95,16 +71,12 @@ QueryCacheDatabaseEntry::QueryCacheDatabaseEntry()
9571

9672
/// @brief destroy a database-specific cache
9773
QueryCacheDatabaseEntry::~QueryCacheDatabaseEntry() {
98-
for (auto& it : _entriesByHash) {
99-
tryDelete(it.second);
100-
}
101-
10274
_entriesByHash.clear();
10375
_entriesByCollection.clear();
10476
}
10577

10678
/// @brief lookup a query result in the database-specific cache
107-
QueryCacheResultEntry* QueryCacheDatabaseEntry::lookup(
79+
std::shared_ptr<QueryCacheResultEntry> QueryCacheDatabaseEntry::lookup(
10880
uint64_t hash, QueryString const& queryString) {
10981
auto it = _entriesByHash.find(hash);
11082

@@ -123,26 +95,20 @@ QueryCacheResultEntry* QueryCacheDatabaseEntry::lookup(
12395
}
12496

12597
// found an entry
126-
auto entry = (*it).second;
127-
128-
// mark the entry as being used so noone else can delete it while it is in use
129-
entry->use();
130-
131-
return entry;
98+
return (*it).second;
13299
}
133100

134101
/// @brief store a query result in the database-specific cache
135102
void QueryCacheDatabaseEntry::store(uint64_t hash,
136-
QueryCacheResultEntry* entry) {
103+
std::shared_ptr<QueryCacheResultEntry> entry) {
137104
// insert entry into the cache
138105
if (!_entriesByHash.emplace(hash, entry).second) {
139106
// remove previous entry
140107
auto it = _entriesByHash.find(hash);
141108
TRI_ASSERT(it != _entriesByHash.end());
142109
auto previous = (*it).second;
143-
unlink(previous);
110+
unlink(previous.get());
144111
_entriesByHash.erase(it);
145-
tryDelete(previous);
146112

147113
// and insert again
148114
_entriesByHash.emplace(hash, entry);
@@ -177,19 +143,19 @@ void QueryCacheDatabaseEntry::store(uint64_t hash,
177143
TRI_ASSERT(it != _entriesByHash.end());
178144
auto previous = (*it).second;
179145
_entriesByHash.erase(it);
180-
unlink(previous);
181-
tryDelete(previous);
146+
unlink(previous.get());
182147
throw;
183148
}
184149

185-
link(entry);
150+
link(entry.get());
186151

187-
enforceMaxResults(MaxResults);
152+
size_t maxResults = MaxResults.load();
153+
enforceMaxResults(maxResults);
188154

189-
TRI_ASSERT(_numElements <= MaxResults);
155+
TRI_ASSERT(_numElements <= maxResults);
190156
TRI_ASSERT(_head != nullptr);
191157
TRI_ASSERT(_tail != nullptr);
192-
TRI_ASSERT(_tail == entry);
158+
TRI_ASSERT(_tail == entry.get());
193159
TRI_ASSERT(entry->_next == nullptr);
194160
}
195161

@@ -217,13 +183,10 @@ void QueryCacheDatabaseEntry::invalidate(std::string const& collection) {
217183
if (it3 != _entriesByHash.end()) {
218184
// remove entry from the linked list
219185
auto entry = (*it3).second;
220-
unlink(entry);
186+
unlink(entry.get());
221187

222188
// erase it from hash table
223189
_entriesByHash.erase(it3);
224-
225-
// delete the object itself
226-
tryDelete(entry);
227190
}
228191
}
229192

@@ -241,15 +204,9 @@ void QueryCacheDatabaseEntry::enforceMaxResults(size_t value) {
241204
auto it = _entriesByHash.find(head->_hash);
242205
TRI_ASSERT(it != _entriesByHash.end());
243206
_entriesByHash.erase(it);
244-
tryDelete(head);
245207
}
246208
}
247209

248-
/// @brief check whether the element can be destroyed, and delete it if yes
249-
void QueryCacheDatabaseEntry::tryDelete(QueryCacheResultEntry* e) {
250-
e->tryDelete();
251-
}
252-
253210
/// @brief unlink the result entry from the list
254211
void QueryCacheDatabaseEntry::unlink(QueryCacheResultEntry* e) {
255212
if (e->_prev != nullptr) {
@@ -312,7 +269,7 @@ VPackBuilder QueryCache::properties() {
312269
VPackBuilder builder;
313270
builder.openObject();
314271
builder.add("mode", VPackValue(modeString(mode())));
315-
builder.add("maxResults", VPackValue(MaxResults));
272+
builder.add("maxResults", VPackValue(MaxResults.load()));
316273
builder.close();
317274

318275
return builder;
@@ -323,7 +280,7 @@ void QueryCache::properties(std::pair<std::string, size_t>& result) {
323280
MUTEX_LOCKER(mutexLocker, _propertiesLock);
324281

325282
result.first = modeString(mode());
326-
result.second = MaxResults;
283+
result.second = MaxResults.load();
327284
}
328285

329286
/// @brief set the cache properties
@@ -361,8 +318,8 @@ std::string QueryCache::modeString(QueryCacheMode mode) {
361318
}
362319

363320
/// @brief lookup a query result in the cache
364-
QueryCacheResultEntry* QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
365-
QueryString const& queryString) {
321+
std::shared_ptr<QueryCacheResultEntry> QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
322+
QueryString const& queryString) {
366323
auto const part = getPart(vocbase);
367324
READ_LOCKER(readLocker, _entriesLock[part]);
368325

@@ -379,7 +336,7 @@ QueryCacheResultEntry* QueryCache::lookup(TRI_vocbase_t* vocbase, uint64_t hash,
379336
/// @brief store a query in the cache
380337
/// if the call is successful, the cache has taken over ownership for the
381338
/// query result!
382-
QueryCacheResultEntry* QueryCache::store(
339+
std::shared_ptr<QueryCacheResultEntry> QueryCache::store(
383340
TRI_vocbase_t* vocbase, uint64_t hash, QueryString const& queryString,
384341
std::shared_ptr<VPackBuilder> result,
385342
std::vector<std::string> const& collections) {
@@ -392,7 +349,7 @@ QueryCacheResultEntry* QueryCache::store(
392349
auto const part = getPart(vocbase);
393350

394351
// create the cache entry outside the lock
395-
auto entry = std::make_unique<QueryCacheResultEntry>(
352+
auto entry = std::make_shared<QueryCacheResultEntry>(
396353
hash, queryString, result, collections);
397354

398355
WRITE_LOCKER(writeLocker, _entriesLock[part]);
@@ -407,8 +364,8 @@ QueryCacheResultEntry* QueryCache::store(
407364
}
408365

409366
// store cache entry
410-
(*it).second->store(hash, entry.get());
411-
return entry.release();
367+
(*it).second->store(hash, entry);
368+
return entry;
412369
}
413370

414371
/// @brief invalidate all queries for the given collections
@@ -512,16 +469,19 @@ void QueryCache::invalidate(unsigned int part) {
512469
}
513470

514471
/// @brief sets the maximum number of results in each per-database cache
472+
/// is called under the mutex
515473
void QueryCache::setMaxResults(size_t value) {
516474
if (value == 0) {
517475
return;
518476
}
519477

520-
if (value > MaxResults) {
478+
size_t maxResults = MaxResults.load();
479+
480+
if (value > maxResults) {
521481
enforceMaxResults(value);
522482
}
523483

524-
MaxResults = value;
484+
maxResults = value;
525485
}
526486

527487
/// @brief sets the caching mode

‎arangod/Aql/QueryCache.h

Lines changed: 8 additions & 42 deletions
Original file line 1241 numberDiff line numberDiff line change
@@ -48,15 +48,6 @@ struct QueryCacheResultEntry {
4848

4949
~QueryCacheResultEntry() = default;
5050

51-
/// @brief check whether the element can be destroyed, and delete it if yes
52-
void tryDelete();
53-
54-
/// @brief use the element, so it cannot be deleted meanwhile
55-
void use();
56-
57-
/// @brief unuse the element, so it can be deleted if required
58-
void unuse();
59-
6051
uint64_t const _hash;
6152
std::string const _queryString;
6253
std::shared_ptr<arangodb::velocypack::Builder> _queryResult;
@@ -67,28 +58,6 @@ struct QueryCacheResultEntry {
6758
std::atomic<uint32_t> _deletionRequested;
6859
};
6960

70-
class QueryCacheResultEntryGuard {
71-
QueryCacheResultEntryGuard(QueryCacheResultEntryGuard const&) = delete;
72-
QueryCacheResultEntryGuard& operator=(QueryCacheResultEntryGuard const&) =
73-
delete;
74-
QueryCacheResultEntryGuard() = delete;
75-
76-
public:
77-
explicit QueryCacheResultEntryGuard(QueryCacheResultEntry* entry)
78-
: _entry(entry) {}
79-
80-
~QueryCacheResultEntryGuard() {
81-
if (_entry != nullptr) {
82-
_entry->unuse();
83-
}
84-
}
85-
86-
QueryCacheResultEntry* get() { return _entry; }
87-
88-
private:
89-
QueryCacheResultEntry* _entry;
90-
};
91-
9261
struct QueryCacheDatabaseEntry {
9362
QueryCacheDatabaseEntry(QueryCacheDatabaseEntry const&) = delete;
9463
QueryCacheDatabaseEntry& operator=(QueryCacheDatabaseEntry const&) = delete;
@@ -100,10 +69,10 @@ struct QueryCacheDatabaseEntry {
10069
~QueryCacheDatabaseEntry();
10170

10271
/// @brief lookup a query result in the database-specific cache
103-
QueryCacheResultEntry* lookup(uint64_t, QueryString const&);
72+
std::shared_ptr<QueryCacheResultEntry> lookup(uint64_t, QueryString const&);
10473

10574
/// @brief store a query result in the database-specific cache
106-
void store(uint64_t, QueryCacheResultEntry*);
75+
void store(uint64_t, std::shared_ptr<QueryCacheResultEntry>);
10776

10877
/// @brief invalidate all entries for the given collections in the
10978
/// database-specific cache
@@ -116,17 +85,14 @@ struct QueryCacheDatabaseEntry {
11685
/// @brief enforce maximum number of results
11786
void enforceMaxResults(size_t);
11887

119-
/// @brief check whether the element can be destroyed, and delete it if yes
120-
void tryDelete(QueryCacheResultEntry*);
121-
12288
/// @brief unlink the result entry from the list
12389
void unlink(QueryCacheResultEntry*);
12490

12591
/// @brief link the result entry to the end of the list
12692
void link(QueryCacheResultEntry*);
12793

12894
/// @brief hash table that maps query hashes to query results
129-
std::unordered_map<uint64_t, QueryCacheResultEntry*> _entriesByHash;
95+
std::unordered_map<uint64_t, std::shared_ptr<QueryCacheResultEntry>> _entriesByHash;
13096

13197
/// @brief hash table that contains all collection-specific query results
13298
/// maps from collection names to a set of query results as defined in
@@ -177,12 +143,12 @@ class QueryCache {
177143
static std::string modeString(QueryCacheMode);
178144

179145
/// @brief lookup a query result in the cache
180-
QueryCacheResultEntry* lookup(TRI_vocbase_t*, uint64_t, QueryString const&);
146+
std::shared_ptr<QueryCacheResultEntry> lookup(TRI_vocbase_t*, uint64_t, QueryString const&);
181147

182148
/// @brief store a query in the cache
183149
/// if the call is successful, the cache has taken over ownership for the
184150
/// query result!
185-
QueryCacheResultEntry* store(TRI_vocbase_t*, uint64_t, QueryString const&,
151+
std::shared_ptr<QueryCacheResultEntry> store(TRI_vocbase_t*, uint64_t, QueryString const&,
186152
std::shared_ptr<arangodb::velocypack::Builder>,
187153
std::vector<std::string> const&);
188154

@@ -211,16 +177,16 @@ class QueryCache {
211177
/// note that the caller of this method must hold the write lock
212178
void invalidate(unsigned int);
213179

214-
/// @brief sets the maximum number of elements in the cache
215-
void setMaxResults(size_t);
216-
217180
/// @brief enable or disable the query cache
218181
void setMode(QueryCacheMode);
219182

220183
/// @brief enable or disable the query cache
221184
void setMode(std::string const&);
222185

223186
private:
187+
/// @brief sets the maximum number of elements in the cache
188+
void setMaxResults(size_t);
189+
224190
/// @brief number of R/W locks for the query cache
225191
static uint64_t const NumberOfParts = 8;
226192

0 commit comments

Comments
 (0)
0