Skip to content

Commit 2151eaa

Browse files
wskozlowskiWojtek
wskozlowski
and
Wojtek
authored
bugfixes (#780)
Co-authored-by: Wojtek <wskozlowski@dbzero.io>
1 parent 98d733e commit 2151eaa

5 files changed

Lines changed: 123 additions & 42 deletions

File tree

‎python_tests/test_atomic.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,7 +1113,9 @@ def test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_
11131113
run_pytest_child(
11141114
"python_tests/test_atomic.py::test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_rollback_child",
11151115
env_flag="DB0_ATOMIC_LEAKED_FIND_ITERATOR_CHILD",
1116+
timeout=30,
11161117
failure_label="atomic leaked find iterator child",
1118+
pytest_args=("-o", "faulthandler_timeout=10"),
11171119
)
11181120

11191121

@@ -1122,6 +1124,24 @@ def test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_
11221124
reason="executed by test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_rollback",
11231125
)
11241126
def test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_rollback_child(db0_no_autocommit):
1127+
def advance(iterator, count):
1128+
values = []
1129+
for _ in range(count):
1130+
try:
1131+
values.append(next(iterator).value)
1132+
except StopIteration:
1133+
break
1134+
return values
1135+
1136+
def assert_no_rolled_back_values(values, iteration):
1137+
assert not any(
1138+
isinstance(value, tuple)
1139+
and len(value) >= 2
1140+
and value[0] == "rolled-back"
1141+
and value[1] == iteration
1142+
for value in values
1143+
)
1144+
11251145
first = MemoTestClass("first")
11261146
db0.commit()
11271147

@@ -1143,6 +1163,38 @@ def test_leaked_find_iterator_advanced_inside_canceled_atomic_repositions_after_
11431163
with pytest.raises(StopIteration):
11441164
next(iterator)
11451165

1166+
rng = random.Random(0xDB0F17D)
1167+
leaked_iterators = [iterator]
1168+
seeds = [MemoTestClass(("seed", index)) for index in range(32)]
1169+
db0.commit()
1170+
1171+
iteration_count = int(os.environ.get("DB0_ATOMIC_LEAKED_FIND_ITERATOR_ITERATIONS", "100"))
1172+
for iteration in range(iteration_count):
1173+
iterator = iter(db0.find(MemoTestClass))
1174+
advance(iterator, rng.randrange(1, len(seeds) + 5))
1175+
1176+
with db0.atomic() as atomic:
1177+
for offset in range(rng.randrange(1, 8)):
1178+
MemoTestClass(("rolled-back", iteration, offset))
1179+
1180+
values = advance(iterator, rng.randrange(1, len(seeds) + 8))
1181+
assert_no_rolled_back_values(values, iteration)
1182+
1183+
if rng.random() < 0.5:
1184+
list(db0.find(MemoTestClass))
1185+
1186+
atomic.cancel()
1187+
1188+
values = advance(iterator, rng.randrange(1, 16))
1189+
assert_no_rolled_back_values(values, iteration)
1190+
1191+
leaked_iterators.append(iterator)
1192+
if len(leaked_iterators) > 64:
1193+
del leaked_iterators[:32]
1194+
1195+
if iteration % 25 == 0:
1196+
gc.collect()
1197+
11461198

11471199
@pytest.mark.skip(reason=ATOMIC_LEAKED_ITERATOR_REPRO_SKIP)
11481200
def test_leaked_iterator_advanced_inside_canceled_atomic_repositions_after_rollback(run_pytest_child):

‎src/dbzero/core/collections/full_text/FT_BaseIndex.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ namespace db0
5656
}
5757
return std::unique_ptr<FT_Iterator<KeyT> >(
5858
new FT_IndexIterator<ListT, KeyT, IndexKeyT>(
59-
*inverted_list_ptr, direction, key, std::move(index_key_sequence))
59+
*inverted_list_ptr, direction, key, std::move(index_key_sequence),
60+
[this, key]() { return this->tryGetExistingInvertedList(key); })
6061
);
6162
}
6263

@@ -79,7 +80,8 @@ namespace db0
7980
// key inverted index
8081
factory.add(std::unique_ptr<FT_Iterator<KeyT> >(
8182
new FT_IndexIterator<ListT, KeyT, IndexKeyT>(
82-
*inverted_list_ptr, -1, key, std::move(index_key_sequence)))
83+
*inverted_list_ptr, -1, key, std::move(index_key_sequence),
84+
[this, key]() { return this->tryGetExistingInvertedList(key); }))
8385
);
8486
return true;
8587
}

‎src/dbzero/core/collections/full_text/FT_BaseIndex.hpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,11 @@ namespace db0
7777
// build query tree
7878
for (const auto &inverted_list: inverted_lists) {
7979
// key inverted index
80+
auto key = inverted_list.first;
8081
factory.add(std::unique_ptr<FT_Iterator<KeyT> >(
81-
new FT_IndexIterator<ListT, KeyT>(
82-
*inverted_list.second, -1, inverted_list.first, {}))
82+
new FT_IndexIterator<ListT, KeyT, IndexKeyT>(
83+
*inverted_list.second, -1, key, {},
84+
[this, key]() { return this->tryGetExistingInvertedList(key); }))
8385
);
8486
}
8587
return result;

‎src/dbzero/core/collections/full_text/FT_IndexIterator.hpp‎

Lines changed: 62 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include <memory>
1212
#include <optional>
1313
#include <vector>
14+
#include <functional>
1415

1516
namespace db0
1617

@@ -27,6 +28,8 @@ namespace db0
2728
using self_t = FT_IndexIterator<bindex_t, key_t, IndexKeyT>;
2829
using super_t = FT_Iterator<key_t>;
2930
using iterator = typename bindex_t::joinable_const_iterator;
31+
using data_ptr = std::shared_ptr<const bindex_t>;
32+
using data_resolver = std::function<data_ptr()>;
3033

3134
// data is the live b-index view used to build the native iterator.
3235
// detach/reattach rebuilds only the native iterator state from this view,
@@ -38,7 +41,7 @@ namespace db0
3841
// tag iterators it stores the root-to-leaf tag key path so deserialize
3942
// can traverse from the root TagIndex instead of persisting child index addresses.
4043
FT_IndexIterator(const bindex_t &data, int direction, std::optional<IndexKeyT> index_key = {},
41-
std::vector<IndexKeyT> &&index_key_sequence = {});
44+
std::vector<IndexKeyT> &&index_key_sequence = {}, data_resolver resolver = {});
4245

4346
FT_IndexIterator(bindex_t &&data, int direction, std::optional<IndexKeyT> index_key = {},
4447
std::vector<IndexKeyT> &&index_key_sequence = {});
@@ -47,7 +50,8 @@ namespace db0
4750
* Construct over already initialized simple iterator
4851
*/
4952
FT_IndexIterator(const bindex_t &data, int direction, const iterator &it,
50-
std::optional<IndexKeyT> index_key = {}, std::vector<IndexKeyT> &&index_key_sequence = {});
53+
std::optional<IndexKeyT> index_key = {}, std::vector<IndexKeyT> &&index_key_sequence = {},
54+
data_resolver resolver = {});
5155

5256
virtual ~FT_IndexIterator() = default;
5357

@@ -93,8 +97,15 @@ namespace db0
9397
void getSignature(std::vector<std::byte> &) const override;
9498

9599
protected:
96-
std::unique_ptr<bindex_t> m_owned_data;
97-
const bindex_t *m_data = nullptr;
100+
// Current b-index view used by m_iterator. Resolver-backed iterators clear it
101+
// on detach so reattachment cannot accidentally reuse a stale MorphingBIndex
102+
// view. Temporary in-memory indexes own the moved b-index through this pointer.
103+
data_ptr m_data;
104+
// Optional reattachment hook. Persistent tag iterators use it to re-resolve
105+
// the current inverted-list b-index after rollback/morphing may have changed
106+
// the underlying MorphingBIndex address/type. Temporary indexes resolve to
107+
// their owned m_data; plain borrowed indexes leave it empty.
108+
data_resolver m_data_resolver;
98109
const int m_direction;
99110
// underlying native iterator (joinable_const_iterator)
100111
mutable iterator m_iterator;
@@ -108,11 +119,14 @@ namespace db0
108119
const std::vector<IndexKeyT> m_index_key_sequence;
109120

110121
FT_IndexIterator(std::uint64_t uid, const bindex_t &data, int direction,
111-
std::optional<IndexKeyT> index_key = {}, std::vector<IndexKeyT> &&index_key_sequence = {});
122+
std::optional<IndexKeyT> index_key = {}, std::vector<IndexKeyT> &&index_key_sequence = {},
123+
data_resolver resolver = {});
112124

113125
FT_IndexIterator(std::uint64_t uid, bindex_t &&data, int direction,
114126
std::optional<IndexKeyT> index_key = {}, std::vector<IndexKeyT> &&index_key_sequence = {});
115127

128+
static data_ptr borrowData(const bindex_t &data);
129+
116130
/**
117131
* Get valid iterator after detach
118132
* @return
@@ -132,10 +146,12 @@ namespace db0
132146

133147
template <typename bindex_t, typename key_t, typename IndexKeyT>
134148
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::FT_IndexIterator(const bindex_t &data, int direction,
135-
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence)
136-
: m_data(&data)
149+
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence,
150+
data_resolver resolver)
151+
: m_data(borrowData(data))
152+
, m_data_resolver(std::move(resolver))
137153
, m_direction(direction)
138-
, m_iterator(m_data->beginJoin(direction))
154+
, m_iterator(data.beginJoin(direction))
139155
, m_index_key(index_key)
140156
, m_index_key_sequence(std::move(index_key_sequence))
141157
{
@@ -144,8 +160,8 @@ namespace db0
144160
template <typename bindex_t, typename key_t, typename IndexKeyT>
145161
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::FT_IndexIterator(bindex_t &&data, int direction,
146162
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence)
147-
: m_owned_data(std::make_unique<bindex_t>(std::move(data)))
148-
, m_data(m_owned_data.get())
163+
: m_data(std::make_shared<bindex_t>(std::move(data)))
164+
, m_data_resolver([data = m_data]() { return data; })
149165
, m_direction(direction)
150166
, m_iterator(m_data->beginJoin(direction))
151167
, m_index_key(index_key)
@@ -155,8 +171,10 @@ namespace db0
155171

156172
template <typename bindex_t, typename key_t, typename IndexKeyT>
157173
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::FT_IndexIterator(const bindex_t &data, int direction, const iterator &it,
158-
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence)
159-
: m_data(&data)
174+
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence,
175+
data_resolver resolver)
176+
: m_data(borrowData(data))
177+
, m_data_resolver(std::move(resolver))
160178
, m_direction(direction)
161179
, m_iterator(it)
162180
, m_index_key(index_key)
@@ -166,11 +184,13 @@ namespace db0
166184

167185
template <typename bindex_t, typename key_t, typename IndexKeyT>
168186
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::FT_IndexIterator(std::uint64_t uid, const bindex_t &data, int direction,
169-
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence)
187+
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence,
188+
data_resolver resolver)
170189
: FT_Iterator<key_t>(uid)
171-
, m_data(&data)
190+
, m_data(borrowData(data))
191+
, m_data_resolver(std::move(resolver))
172192
, m_direction(direction)
173-
, m_iterator(m_data->beginJoin(direction))
193+
, m_iterator(data.beginJoin(direction))
174194
, m_index_key(index_key)
175195
, m_index_key_sequence(std::move(index_key_sequence))
176196
{
@@ -180,8 +200,8 @@ namespace db0
180200
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::FT_IndexIterator(std::uint64_t uid, bindex_t &&data, int direction,
181201
std::optional<IndexKeyT> index_key, std::vector<IndexKeyT> &&index_key_sequence)
182202
: FT_Iterator<key_t>(uid)
183-
, m_owned_data(std::make_unique<bindex_t>(std::move(data)))
184-
, m_data(m_owned_data.get())
203+
, m_data(std::make_shared<bindex_t>(std::move(data)))
204+
, m_data_resolver([data = m_data]() { return data; })
185205
, m_direction(direction)
186206
, m_iterator(m_data->beginJoin(direction))
187207
, m_index_key(index_key)
@@ -248,15 +268,9 @@ namespace db0
248268

249269
template <typename bindex_t, typename key_t, typename IndexKeyT>
250270
std::unique_ptr<FT_Iterator<key_t> > FT_IndexIterator<bindex_t, key_t, IndexKeyT>::beginTyped(int direction) const {
251-
if (m_owned_data) {
252-
return std::unique_ptr<FT_Iterator<key_t> >(
253-
new FT_IndexIterator(this->m_uid, bindex_t(*m_data), direction, this->m_index_key,
254-
std::vector<IndexKeyT>(this->m_index_key_sequence))
255-
);
256-
}
257271
return std::unique_ptr<FT_Iterator<key_t> >(
258272
new FT_IndexIterator(this->m_uid, *m_data, direction, this->m_index_key,
259-
std::vector<IndexKeyT>(this->m_index_key_sequence))
273+
std::vector<IndexKeyT>(this->m_index_key_sequence), this->m_data_resolver)
260274
);
261275
}
262276

@@ -298,6 +312,9 @@ namespace db0
298312
this->m_detach_key = *(this->m_iterator);
299313
}
300314
this->m_iterator.reset();
315+
if (this->m_data_resolver) {
316+
this->m_data.reset();
317+
}
301318
this->m_is_detached = true;
302319
}
303320
}
@@ -306,26 +323,37 @@ namespace db0
306323
typename FT_IndexIterator<bindex_t, key_t, IndexKeyT>::iterator &FT_IndexIterator<bindex_t, key_t, IndexKeyT>::getIterator()
307324
{
308325
if (m_is_detached) {
309-
try {
310-
m_force_end = false;
311-
m_iterator = m_data->beginJoin(m_direction);
312-
if (m_detach_key) {
313-
if (!m_iterator.join(*m_detach_key, m_direction)) {
314-
m_iterator.stop();
315-
m_force_end = true;
316-
}
317-
} else {
326+
m_force_end = false;
327+
if (m_data_resolver) {
328+
m_data = m_data_resolver();
329+
}
330+
if (!m_data) {
331+
m_force_end = true;
332+
m_is_detached = false;
333+
return m_iterator;
334+
}
335+
m_iterator = m_data->beginJoin(m_direction);
336+
if (m_detach_key) {
337+
if (!m_iterator.join(*m_detach_key, m_direction)) {
318338
m_iterator.stop();
319339
m_force_end = true;
320340
}
321-
} catch (...) {
341+
} else {
342+
m_iterator.stop();
322343
m_force_end = true;
323344
}
324345
m_is_detached = false;
325346
}
326347
return m_iterator;
327348
}
328349

350+
template <typename bindex_t, typename key_t, typename IndexKeyT>
351+
typename FT_IndexIterator<bindex_t, key_t, IndexKeyT>::data_ptr
352+
FT_IndexIterator<bindex_t, key_t, IndexKeyT>::borrowData(const bindex_t &data)
353+
{
354+
return data_ptr(&data, [](const bindex_t *) {});
355+
}
356+
329357
template <typename bindex_t, typename key_t, typename IndexKeyT>
330358
const typename FT_IndexIterator<bindex_t, key_t, IndexKeyT>::iterator &FT_IndexIterator<bindex_t, key_t, IndexKeyT>::getIterator() const
331359
{

‎src/dbzero/core/collections/range_tree/FT_BoundIterator.hpp‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,7 @@ namespace db0
132132
template <typename KeyT, typename ValueT, typename IndexT>
133133
std::unique_ptr<FT_Iterator<ValueT> > FT_BoundIterator<KeyT, ValueT, IndexT>::beginTyped(int direction) const
134134
{
135-
if (super_t::m_owned_data) {
136-
return std::unique_ptr<FT_Iterator<ValueT> >(new self_t(IndexT(*super_t::m_data), direction, m_key_range));
137-
}
138-
return std::unique_ptr<FT_Iterator<ValueT> >(new self_t(*super_t::m_data, direction, m_key_range));
135+
return std::unique_ptr<FT_Iterator<ValueT> >(new self_t(IndexT(*super_t::m_data), direction, m_key_range));
139136
}
140137

141138
}

0 commit comments

Comments
 (0)