Skip to content

Commit

Permalink
Fix assertion failures in debug build
Browse files Browse the repository at this point in the history
- Don't assert out when S3 is already initialized
- Add vfs.terminate calls to match vfs->init in test sections
- Add missing status check in tiledb_vfs_ls
  • Loading branch information
ihnorton committed Mar 11, 2020
1 parent 02a8584 commit 43c7642
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 0 deletions.
9 changes: 9 additions & 0 deletions test/src/unit-vfs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,13 @@ TEST_CASE("VFS: Test read batching", "[vfs]") {
ThreadPool thread_pool;
REQUIRE(thread_pool.init(4).ok());
std::vector<std::future<Status>> tasks;
REQUIRE(vfs->terminate().ok());

SECTION("- Default config") {
// Check reading in one batch: single read operation.
std::memset(data_read, 0, nelts * sizeof(uint32_t));
batches.emplace_back(0, data_read, nelts * sizeof(uint32_t));
REQUIRE(vfs->init(nullptr, nullptr).ok());
REQUIRE(vfs->read_all(testfile, batches, &thread_pool, &tasks).ok());
REQUIRE(thread_pool.wait_all(tasks).ok());
tasks.clear();
Expand Down Expand Up @@ -113,6 +115,7 @@ TEST_CASE("VFS: Test read batching", "[vfs]") {
REQUIRE(
stats::all_stats.counter_vfs_read_total_bytes ==
nelts * sizeof(uint32_t));
REQUIRE(vfs->terminate().ok());
}

SECTION("- Reduce min batch size and min batch gap") {
Expand Down Expand Up @@ -193,6 +196,7 @@ TEST_CASE("VFS: Test read batching", "[vfs]") {
CHECK(
stats::all_stats.counter_vfs_read_total_bytes ==
nelts * sizeof(uint32_t));
REQUIRE(vfs->terminate().ok());
}

SECTION("- Reduce min batch gap but not min batch size") {
Expand All @@ -216,8 +220,11 @@ TEST_CASE("VFS: Test read batching", "[vfs]") {
CHECK(
stats::all_stats.counter_vfs_read_total_bytes ==
nelts * sizeof(uint32_t));
REQUIRE(vfs->terminate().ok());
}

Config default_config, vfs_config;
REQUIRE(vfs->init(&default_config, &vfs_config).ok());
REQUIRE(vfs->is_file(testfile, &exists).ok());
if (exists)
REQUIRE(vfs->remove_file(testfile).ok());
Expand Down Expand Up @@ -317,6 +324,7 @@ TEST_CASE("VFS: Test long posix paths", "[vfs]") {
}

REQUIRE(vfs->remove_dir(URI(tmpdir_base)).ok());
REQUIRE(vfs->terminate().ok());
}

#endif
Expand Down Expand Up @@ -461,5 +469,6 @@ TEST_CASE("VFS: URI semantics", "[vfs][uri]") {
} else {
REQUIRE(vfs.remove_dir(root).ok());
}
vfs.terminate();
}
}
3 changes: 3 additions & 0 deletions tiledb/sm/c_api/tiledb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4015,6 +4015,9 @@ int32_t tiledb_vfs_ls(
std::vector<tiledb::sm::URI> children;
auto st = vfs->vfs_->ls(tiledb::sm::URI(path), &children);

if (!st.ok())
return TILEDB_ERR;

// Apply the callback to every child
int rc = 1;
for (const auto& uri : children) {
Expand Down
4 changes: 4 additions & 0 deletions tiledb/sm/filesystem/s3.cc
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ S3::~S3() {
/* ********************************* */

Status S3::init(const Config& config, ThreadPool* const thread_pool) {
// already initialized
if (state_ == State::DISCONNECTED)
return Status::Ok();

assert(state_ == State::UNINITIALIZED);

if (thread_pool == nullptr) {
Expand Down
3 changes: 3 additions & 0 deletions tiledb/sm/filesystem/s3_thread_pool_executor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ S3ThreadPoolExecutor::~S3ThreadPoolExecutor() {
}

Status S3ThreadPoolExecutor::Stop() {
if (state_ == State::STOPPED)
return Status::Ok();

Status ret_st = Status::Ok();

std::unique_lock<std::mutex> lock_guard(lock_);
Expand Down

0 comments on commit 43c7642

Please sign in to comment.