Skip to content

Commit 892a8bb

Browse files
teo-tsirpanisteo-tsirpanis
authored
Fix dangling memory tracker references in the buffer and buffer list C API handles et. al. (#5305)
[SC-55087](https://app.shortcut.com/tiledb-inc/story/55087/do-not-show-message-dialog-on-debug-assertion-failures-in-tiledb-unit-on-windows-ci) Fixes #5295 Several tests allocate a `tiledb_buffer_t*` but never free it. When the context that created the buffer gets freed, the buffer's memory tracker gets freed, which causes a debug assertion to fail because that tracker had some outstanding memory allocated. This PR fixes it by adding a `shared_ptr<MemoryTracker>` field in `tiledb_buffer(_list)_handle_t`, ensuring the memory tracker does not outlive the C API handles. While the assertion failures could be avoided by patching the buffer handle leaks, freeing a buffer handle (or any kind of handle) after its context should not fail unless in extreme cases. Memory leaks in test code will be addressed in a subsequent PR (opened SC-55086 to track this work). I also updated `tiledb_unit` to not display a message box on assertion failures when running on Windows CI, which caused timeouts similar to #4250. We should eventually do this to all Catch2 tests. --- TYPE: NO_HISTORY
1 parent a35426a commit 892a8bb

File tree

9 files changed

+70
-76
lines changed

9 files changed

+70
-76
lines changed

‎test/src/test-cppapi-consolidation-plan.cc‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -188,17 +188,9 @@ tiledb::sm::ConsolidationPlan CppConsolidationPlanFx::call_handler(
188188
const Array& array,
189189
tiledb::sm::SerializationType stype) {
190190
auto req_buf = tiledb_buffer_handle_t::make_handle(
191-
ctx_.ptr()
192-
.get()
193-
->resources()
194-
.serialization_memory_tracker()
195-
->get_resource(tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
191+
ctx_.ptr().get()->resources().serialization_memory_tracker());
196192
auto resp_buf = tiledb_buffer_handle_t::make_handle(
197-
ctx_.ptr()
198-
.get()
199-
->resources()
200-
.serialization_memory_tracker()
201-
->get_resource(tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
193+
ctx_.ptr().get()->resources().serialization_memory_tracker());
202194

203195
tiledb::sm::serialization::serialize_consolidation_plan_request(
204196
fragment_size, cfg_.ptr()->config(), stype, req_buf->buffer());

‎test/src/unit-request-handlers.cc‎

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,9 @@ TEST_CASE_METHOD(
214214
auto array = tiledb::Array(ctx, uri_.to_string(), TILEDB_READ);
215215
auto stype = TILEDB_CAPNP;
216216
auto req_buf = tiledb_buffer_handle_t::make_handle(
217-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
218-
MemoryType::SERIALIZATION_BUFFER));
217+
ctx.ptr()->resources().serialization_memory_tracker());
219218
auto resp_buf = tiledb_buffer_handle_t::make_handle(
220-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
221-
MemoryType::SERIALIZATION_BUFFER));
219+
ctx.ptr()->resources().serialization_memory_tracker());
222220

223221
auto rval = tiledb_handle_load_array_schema_request(
224222
nullptr,
@@ -325,11 +323,9 @@ TEST_CASE_METHOD(
325323
auto array = tiledb::Array(ctx, uri_.to_string(), TILEDB_READ);
326324
auto stype = TILEDB_CAPNP;
327325
auto req_buf = tiledb_buffer_handle_t::make_handle(
328-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
329-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
326+
ctx.ptr()->resources().serialization_memory_tracker());
330327
auto resp_buf = tiledb_buffer_handle_t::make_handle(
331-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
332-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
328+
ctx.ptr()->resources().serialization_memory_tracker());
333329

334330
auto rval = tiledb_handle_query_plan_request(
335331
nullptr,
@@ -374,11 +370,9 @@ TEST_CASE_METHOD(
374370
auto array = tiledb::Array(ctx, uri_.to_string(), TILEDB_READ);
375371
auto stype = TILEDB_CAPNP;
376372
auto req_buf = tiledb_buffer_handle_t::make_handle(
377-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
378-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
373+
ctx.ptr()->resources().serialization_memory_tracker());
379374
auto resp_buf = tiledb_buffer_handle_t::make_handle(
380-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
381-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
375+
ctx.ptr()->resources().serialization_memory_tracker());
382376

383377
auto rval = tiledb_handle_consolidation_plan_request(
384378
nullptr,
@@ -536,11 +530,9 @@ HandleLoadArraySchemaRequestFx::call_handler(
536530
auto ctx = tiledb::Context();
537531
auto array = tiledb::Array(ctx, uri_.to_string(), TILEDB_READ);
538532
auto req_buf = tiledb_buffer_handle_t::make_handle(
539-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
540-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
533+
ctx.ptr()->resources().serialization_memory_tracker());
541534
auto resp_buf = tiledb_buffer_handle_t::make_handle(
542-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
543-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
535+
ctx.ptr()->resources().serialization_memory_tracker());
544536

545537
serialization::serialize_load_array_schema_request(
546538
cfg_, req, stype, req_buf->buffer());
@@ -591,11 +583,9 @@ QueryPlan HandleQueryPlanRequestFx::call_handler(
591583
auto ctx = tiledb::Context();
592584
auto array = tiledb::Array(ctx, uri_.to_string(), TILEDB_READ);
593585
auto req_buf = tiledb_buffer_handle_t::make_handle(
594-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
595-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
586+
ctx.ptr()->resources().serialization_memory_tracker());
596587
auto resp_buf = tiledb_buffer_handle_t::make_handle(
597-
ctx.ptr()->resources().serialization_memory_tracker()->get_resource(
598-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
588+
ctx.ptr()->resources().serialization_memory_tracker());
599589

600590
serialization::serialize_query_plan_request(
601591
cfg_, query, stype, req_buf->buffer());

‎test/src/unit.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,16 @@ int store_g_vfs(std::string&& vfs, std::vector<std::string> vfs_fs);
1717
} // namespace tiledb
1818

1919
int main(const int argc, char** const argv) {
20+
#if defined(_MSC_VER)
21+
// We disable the following events on abort in CI environments:
22+
// _WRITE_ABORT_MSG: Display message box with Abort, Retry, Ignore
23+
// _CALL_REPORTFAULT: Send an error report to Microsoft
24+
// The second parameter specifies which flags to change, and the first
25+
// the value of these flags.
26+
if (std::getenv("CI") != nullptr) {
27+
_set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT);
28+
}
29+
#endif
2030
Catch::Session session;
2131

2232
// Define acceptable VFS values.

‎tiledb/api/c_api/buffer/buffer_api.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,7 @@ capi_return_t tiledb_buffer_alloc(
4444
ensure_context_is_valid(ctx);
4545
ensure_output_pointer_is_valid(buffer);
4646
*buffer = tiledb_buffer_handle_t::make_handle(
47-
ctx->resources().serialization_memory_tracker()->get_resource(
48-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
47+
ctx->resources().serialization_memory_tracker());
4948
return TILEDB_OK;
5049
}
5150

‎tiledb/api/c_api/buffer/buffer_api_internal.h‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#define TILEDB_CAPI_BUFFER_API_INTERNAL_H
3535

3636
#include "../../c_api_support/handle/handle.h"
37+
#include "tiledb/common/memory_tracker.h"
3738
#include "tiledb/sm/buffer/buffer.h"
3839
#include "tiledb/sm/enums/datatype.h"
3940

@@ -45,26 +46,36 @@ struct tiledb_buffer_handle_t
4546
static constexpr std::string_view object_type_name{"buffer"};
4647

4748
private:
49+
shared_ptr<tiledb::sm::MemoryTracker> memory_tracker_;
4850
tiledb::sm::SerializationBuffer buffer_;
4951
tiledb::sm::Datatype datatype_;
5052

5153
public:
52-
explicit tiledb_buffer_handle_t(decltype(buffer_)::allocator_type allocator)
53-
: buffer_(allocator)
54+
explicit tiledb_buffer_handle_t(
55+
shared_ptr<tiledb::sm::MemoryTracker> memory_tracker)
56+
: memory_tracker_(std::move(memory_tracker))
57+
, buffer_(memory_tracker_->get_resource(
58+
tiledb::sm::MemoryType::SERIALIZATION_BUFFER))
5459
, datatype_(tiledb::sm::Datatype::UINT8) {
5560
}
5661

5762
explicit tiledb_buffer_handle_t(
58-
size_t size, decltype(buffer_)::allocator_type allocator)
59-
: buffer_(size, allocator)
63+
size_t size, shared_ptr<tiledb::sm::MemoryTracker> memory_tracker)
64+
: memory_tracker_(std::move(memory_tracker))
65+
, buffer_(
66+
size,
67+
memory_tracker_->get_resource(
68+
tiledb::sm::MemoryType::SERIALIZATION_BUFFER))
6069
, datatype_(tiledb::sm::Datatype::UINT8) {
6170
}
6271

6372
explicit tiledb_buffer_handle_t(
6473
const void* data,
6574
uint64_t size,
66-
decltype(buffer_)::allocator_type allocator)
67-
: buffer_(allocator)
75+
shared_ptr<tiledb::sm::MemoryTracker> memory_tracker)
76+
: memory_tracker_(std::move(memory_tracker))
77+
, buffer_(memory_tracker_->get_resource(
78+
tiledb::sm::MemoryType::SERIALIZATION_BUFFER))
6879
, datatype_(tiledb::sm::Datatype::UINT8) {
6980
buffer_.assign(
7081
tiledb::sm::SerializationBuffer::NonOwned,

‎tiledb/api/c_api/buffer_list/buffer_list_api.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ capi_return_t tiledb_buffer_list_get_buffer(
7575

7676
// Create a non-owning wrapper of the underlying buffer
7777
*buffer = tiledb_buffer_handle_t::make_handle(
78-
b.data(), b.size(), buffer_list->buffer_list().get_allocator());
78+
b.data(), b.size(), buffer_list->memory_tracker());
7979

8080
return TILEDB_OK;
8181
}
@@ -89,18 +89,14 @@ capi_return_t tiledb_buffer_list_get_total_size(
8989
}
9090

9191
capi_return_t tiledb_buffer_list_flatten(
92-
tiledb_ctx_handle_t* ctx,
93-
tiledb_buffer_list_t* buffer_list,
94-
tiledb_buffer_t** buffer) {
92+
tiledb_buffer_list_t* buffer_list, tiledb_buffer_t** buffer) {
9593
ensure_buffer_list_is_valid(buffer_list);
9694
ensure_output_pointer_is_valid(buffer);
9795

9896
// Create a serialization buffer
9997
const auto nbytes = buffer_list->buffer_list().total_size();
10098
*buffer = tiledb_buffer_t::make_handle(
101-
buffer_list->buffer_list().total_size(),
102-
ctx->resources().serialization_memory_tracker()->get_resource(
103-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
99+
buffer_list->buffer_list().total_size(), buffer_list->memory_tracker());
104100

105101
try {
106102
// Read all into the dest buffer
@@ -164,6 +160,6 @@ CAPI_INTERFACE(
164160
tiledb_ctx_t* ctx,
165161
tiledb_buffer_list_t* buffer_list,
166162
tiledb_buffer_t** buffer) {
167-
return api_entry_with_context<tiledb::api::tiledb_buffer_list_flatten>(
163+
return api_entry_context<tiledb::api::tiledb_buffer_list_flatten>(
168164
ctx, buffer_list, buffer);
169165
}

‎tiledb/api/c_api/buffer_list/buffer_list_api_internal.h‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,10 @@
3434
#define TILEDB_CAPI_BUFFER_LIST_API_INTERNAL_H
3535

3636
#include "../../c_api_support/handle/handle.h"
37+
#include "tiledb/common/memory_tracker.h"
3738
#include "tiledb/sm/buffer/buffer.h"
3839
#include "tiledb/sm/buffer/buffer_list.h"
40+
3941
struct tiledb_buffer_list_handle_t
4042
: public tiledb::api::CAPIHandle<tiledb_buffer_list_handle_t> {
4143
/**
@@ -44,11 +46,14 @@ struct tiledb_buffer_list_handle_t
4446
static constexpr std::string_view object_type_name{"buffer list"};
4547

4648
private:
49+
shared_ptr<tiledb::sm::MemoryTracker> memory_tracker_;
4750
tiledb::sm::BufferList buffer_list_;
4851

4952
public:
50-
explicit tiledb_buffer_list_handle_t(auto&&... args)
51-
: buffer_list_(std::forward<decltype(args)>(args)...) {
53+
explicit tiledb_buffer_list_handle_t(
54+
shared_ptr<tiledb::sm::MemoryTracker> memory_tracker)
55+
: memory_tracker_(std::move(memory_tracker))
56+
, buffer_list_(memory_tracker_) {
5257
}
5358

5459
[[nodiscard]] inline tiledb::sm::BufferList& buffer_list() {
@@ -58,6 +63,11 @@ struct tiledb_buffer_list_handle_t
5863
[[nodiscard]] inline const tiledb::sm::BufferList& buffer_list() const {
5964
return buffer_list_;
6065
}
66+
67+
[[nodiscard]] inline const shared_ptr<tiledb::sm::MemoryTracker>
68+
memory_tracker() const {
69+
return memory_tracker_;
70+
}
6171
};
6272

6373
namespace tiledb::api {

‎tiledb/api/c_api/group/group_api.cc‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -415,8 +415,7 @@ capi_return_t tiledb_serialize_group(
415415

416416
// ALERT: Conditional Handle Construction
417417
auto buf = tiledb_buffer_handle_t::make_handle(
418-
ctx->resources().serialization_memory_tracker()->get_resource(
419-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
418+
ctx->resources().serialization_memory_tracker());
420419

421420
// We're not using throw_if_not_ok here because we have to
422421
// clean up our allocated buffer if serialization fails.
@@ -461,8 +460,7 @@ capi_return_t tiledb_serialize_group_metadata(
461460

462461
// ALERT: Conditional Handle Construction
463462
auto buf = tiledb_buffer_handle_t::make_handle(
464-
ctx->resources().serialization_memory_tracker()->get_resource(
465-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
463+
ctx->resources().serialization_memory_tracker());
466464

467465
// Get metadata to serialize, this will load it if it does not exist
468466
auto metadata = group->group().metadata();

‎tiledb/sm/c_api/tiledb.cc‎

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2344,8 +2344,7 @@ int32_t tiledb_serialize_array(
23442344
return TILEDB_ERR;
23452345

23462346
auto buf = tiledb_buffer_handle_t::make_handle(
2347-
ctx->resources().serialization_memory_tracker()->get_resource(
2348-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2347+
ctx->resources().serialization_memory_tracker());
23492348

23502349
if (SAVE_ERROR_CATCH(
23512350
ctx,
@@ -2436,8 +2435,7 @@ int32_t tiledb_serialize_array_schema(
24362435

24372436
// Create buffer
24382437
auto buf = tiledb_buffer_handle_t::make_handle(
2439-
ctx->resources().serialization_memory_tracker()->get_resource(
2440-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2438+
ctx->resources().serialization_memory_tracker());
24412439

24422440
if (SAVE_ERROR_CATCH(
24432441
ctx,
@@ -2491,8 +2489,7 @@ int32_t tiledb_serialize_array_open(
24912489
}
24922490

24932491
auto buf = tiledb_buffer_handle_t::make_handle(
2494-
ctx->resources().serialization_memory_tracker()->get_resource(
2495-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2492+
ctx->resources().serialization_memory_tracker());
24962493

24972494
if (SAVE_ERROR_CATCH(
24982495
ctx,
@@ -2579,8 +2576,7 @@ int32_t tiledb_serialize_array_schema_evolution(
25792576
return TILEDB_ERR;
25802577

25812578
auto buf = tiledb_buffer_handle_t::make_handle(
2582-
ctx->resources().serialization_memory_tracker()->get_resource(
2583-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2579+
ctx->resources().serialization_memory_tracker());
25842580

25852581
if (SAVE_ERROR_CATCH(
25862582
ctx,
@@ -2798,8 +2794,7 @@ int32_t tiledb_serialize_array_nonempty_domain(
27982794
return TILEDB_ERR;
27992795

28002796
auto buf = tiledb_buffer_handle_t::make_handle(
2801-
ctx->resources().serialization_memory_tracker()->get_resource(
2802-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2797+
ctx->resources().serialization_memory_tracker());
28032798

28042799
if (SAVE_ERROR_CATCH(
28052800
ctx,
@@ -2856,8 +2851,7 @@ int32_t tiledb_serialize_array_non_empty_domain_all_dimensions(
28562851
return TILEDB_ERR;
28572852

28582853
auto buf = tiledb_buffer_handle_t::make_handle(
2859-
ctx->resources().serialization_memory_tracker()->get_resource(
2860-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2854+
ctx->resources().serialization_memory_tracker());
28612855

28622856
if (SAVE_ERROR_CATCH(
28632857
ctx,
@@ -2905,8 +2899,7 @@ int32_t tiledb_serialize_array_max_buffer_sizes(
29052899
return TILEDB_ERR;
29062900

29072901
auto buf = tiledb_buffer_handle_t::make_handle(
2908-
ctx->resources().serialization_memory_tracker()->get_resource(
2909-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2902+
ctx->resources().serialization_memory_tracker());
29102903

29112904
// Serialize
29122905
if (SAVE_ERROR_CATCH(
@@ -2990,8 +2983,7 @@ int32_t tiledb_serialize_array_metadata(
29902983
return TILEDB_ERR;
29912984

29922985
auto buf = tiledb_buffer_handle_t::make_handle(
2993-
ctx->resources().serialization_memory_tracker()->get_resource(
2994-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
2986+
ctx->resources().serialization_memory_tracker());
29952987

29962988
// Get metadata to serialize, this will load it if it does not exist
29972989
sm::Metadata* metadata = nullptr;
@@ -3052,8 +3044,7 @@ int32_t tiledb_serialize_query_est_result_sizes(
30523044
return TILEDB_ERR;
30533045

30543046
auto buf = tiledb_buffer_handle_t::make_handle(
3055-
ctx->resources().serialization_memory_tracker()->get_resource(
3056-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
3047+
ctx->resources().serialization_memory_tracker());
30573048

30583049
if (SAVE_ERROR_CATCH(
30593050
ctx,
@@ -3103,8 +3094,7 @@ int32_t tiledb_serialize_config(
31033094
api::ensure_config_is_valid(config);
31043095

31053096
auto buf = tiledb_buffer_handle_t::make_handle(
3106-
ctx->resources().serialization_memory_tracker()->get_resource(
3107-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
3097+
ctx->resources().serialization_memory_tracker());
31083098

31093099
if (SAVE_ERROR_CATCH(
31103100
ctx,
@@ -3166,8 +3156,7 @@ int32_t tiledb_serialize_fragment_info_request(
31663156
}
31673157

31683158
auto buf = tiledb_buffer_handle_t::make_handle(
3169-
ctx->resources().serialization_memory_tracker()->get_resource(
3170-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
3159+
ctx->resources().serialization_memory_tracker());
31713160

31723161
if (SAVE_ERROR_CATCH(
31733162
ctx,
@@ -3224,8 +3213,7 @@ int32_t tiledb_serialize_fragment_info(
32243213
}
32253214

32263215
auto buf = tiledb_buffer_handle_t::make_handle(
3227-
ctx->resources().serialization_memory_tracker()->get_resource(
3228-
tiledb::sm::MemoryType::SERIALIZATION_BUFFER));
3216+
ctx->resources().serialization_memory_tracker());
32293217

32303218
// Serialize
32313219
if (SAVE_ERROR_CATCH(

0 commit comments

Comments
 (0)