Skip to content

Commit

Permalink
Handle I/O exceptions during rotation and propagate them to the initi…
Browse files Browse the repository at this point in the history
…ating thread
  • Loading branch information
umegane committed Sep 24, 2024
1 parent 8f7f0a6 commit 44854c6
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 15 deletions.
5 changes: 2 additions & 3 deletions include/limestone/api/limestone_exception.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,16 @@ class limestone_io_exception : public limestone_exception {
public:
// Constructor that takes an error message and errno as arguments (int)
explicit limestone_io_exception(const std::string& message, int error_code)
: limestone_exception(format_message(message, error_code), error_code) {}
: limestone_exception(message, error_code) {}

// Constructor that takes an error message and boost::system::error_code as arguments
explicit limestone_io_exception(const std::string& message, const boost::system::error_code& error_code)
: limestone_exception(format_message(message, error_code), error_code.value()) {}
: limestone_exception(message, error_code.value()) {}

// Helper function to format the error message for int error_code
static std::string format_message(const std::string& message, int error_code) {
// Retrieve the system error message corresponding to errno
std::string errno_str = std::strerror(error_code);

// Format the complete error message
return "I/O Error (" + errno_str + "): " + message + " (errno = " + std::to_string(error_code) + ")";
}
Expand Down
7 changes: 6 additions & 1 deletion src/limestone/datastore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,12 @@ void datastore::rotate_epoch_file() {
<< "." << epoch_id_switched_.load();
std::string new_name = ss.str();
boost::filesystem::path new_file = location_ / new_name;
boost::filesystem::rename(epoch_file_path_, new_file);
boost::system::error_code ec;
boost::filesystem::rename(epoch_file_path_, new_file, ec);
if (ec) {
std::string err_msg = "Failed to rename epoch_file from " + epoch_file_path_.string() + " to " + new_file.string();
LOG_AND_THROW_IO_EXCEPTION(err_msg, ec);
}
add_file(new_file);

// create new one
Expand Down
25 changes: 16 additions & 9 deletions src/limestone/limestone_exception_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,39 @@
namespace limestone {

using limestone::api::limestone_exception;
using limestone::api::limestone_io_exception;
using limestone::api::limestone_io_exception;


inline std::string extract_filename(const std::string& path) {
// Use boost::filesystem::path to handle file path and extract filename
boost::filesystem::path p(path);
return p.filename().string();
}

#define THROW_LIMESTONE_EXCEPTION(message) \

Check warning on line 38 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

cppcoreguidelines-macro-usage

function-like macro 'THROW_LIMESTONE_EXCEPTION' used; consider a 'constexpr' template function
{ \
LOG(ERROR) << message; \
throw limestone_exception(std::string(message) + " (at " + std::string(__FILE__) + ":" + std::to_string(__LINE__) + ")"); \
LOG_LP(ERROR) << message; \

Check warning on line 40 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

bugprone-macro-parentheses

macro argument should be enclosed in parentheses
throw limestone_exception(std::string(message) + " (at " + extract_filename(__FILE__) + ":" + std::to_string(__LINE__) + ")"); \
}

#define THROW_LIMESTONE_IO_EXCEPTION(message, error_code) \

Check warning on line 44 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

cppcoreguidelines-macro-usage

function-like macro 'THROW_LIMESTONE_IO_EXCEPTION' used; consider a 'constexpr' template function
{ \
std::string full_message = limestone_io_exception::format_message(message, error_code); \
LOG(ERROR) << full_message; \
throw limestone_io_exception(full_message + " (at " + std::string(__FILE__) + ":" + std::to_string(__LINE__) + ")", error_code); \
LOG_LP(ERROR) << full_message; \
throw limestone_io_exception(full_message + " (at " + extract_filename(__FILE__) + ":" + std::to_string(__LINE__) + ")", error_code); \
}

#define LOG_AND_THROW_EXCEPTION(message) \

Check warning on line 51 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

cppcoreguidelines-macro-usage

function-like macro 'LOG_AND_THROW_EXCEPTION' used; consider a 'constexpr' template function
{ \
LOG(ERROR) << message; \
throw limestone_exception(std::string(message) + " (at " + std::string(__FILE__) + ":" + std::to_string(__LINE__) + ")"); \
LOG_LP(ERROR) << message; \

Check warning on line 53 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

bugprone-macro-parentheses

macro argument should be enclosed in parentheses
throw limestone_exception(std::string(message) + " (at " + extract_filename(__FILE__) + ":" + std::to_string(__LINE__) + ")"); \
}

#define LOG_AND_THROW_IO_EXCEPTION(message, error_code) \

Check warning on line 57 in src/limestone/limestone_exception_helper.h

View workflow job for this annotation

GitHub Actions / Clang-Tidy

cppcoreguidelines-macro-usage

function-like macro 'LOG_AND_THROW_IO_EXCEPTION' used; consider a 'constexpr' template function
{ \
std::string full_message = limestone_io_exception::format_message(message, error_code); \
LOG(ERROR) << full_message; \
throw limestone_io_exception(full_message + " (at " + std::string(__FILE__) + ":" + std::to_string(__LINE__) + ")", error_code); \
LOG_LP(ERROR) << full_message; \
throw limestone_io_exception(full_message + " (at " + extract_filename(__FILE__) + ":" + std::to_string(__LINE__) + ")", error_code); \
}


Expand Down
6 changes: 6 additions & 0 deletions src/limestone/rotation_task.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ rotation_task::rotation_task(datastore& envelope)


void rotation_task::rotate() {
try {
rotation_result final_result;
for (const auto& lc : envelope_.log_channels_) {
boost::system::error_code error;
Expand All @@ -84,6 +85,11 @@ void rotation_task::rotate() {
final_result.set_rotation_end_files(envelope_.get_files());

result_promise_.set_value(final_result);
} catch (const std::exception& e) {
auto ex_ptr = std::current_exception();
std::cerr << "Setting exception in promise" << std::endl;
result_promise_.set_exception(ex_ptr);
}
}

rotation_result rotation_task::wait_for_result() {
Expand Down
33 changes: 33 additions & 0 deletions test/limestone/compaction/rotation_task_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <limestone/api/datastore.h>
#include "test_root.h"
#include "rotation_task.h"
#include "limestone/api/limestone_exception.h"
namespace limestone::testing {

constexpr const char* data_location = "/tmp/rotation_task_test/data_location";
Expand All @@ -30,6 +31,7 @@ using limestone::api::log_channel;
using limestone::api::rotation_task;
using limestone::api::rotation_result;
using limestone::api::rotation_task_helper;
using limestone::api::limestone_exception;

class rotation_task_test : public ::testing::Test {
protected:
Expand Down Expand Up @@ -139,4 +141,35 @@ TEST_F(rotation_task_test, no_task_execution_when_queue_is_empty) {
SUCCEED();
}


TEST_F(rotation_task_test, task_throws_exception) {
auto task = rotation_task_helper::create_and_enqueue_task(*datastore_);

// Force an exception to be thrown by removing the directory
if (system("rm -rf /tmp/rotation_task_test") != 0) {
std::cerr << "Cannot remove directory" << std::endl;
}

// Since the exception is caught in task->rotate(), no exception should be thrown here
task->rotate();

// Check that an exception is thrown and verify its details
try {
rotation_result result = task->wait_for_result();
FAIL() << "Expected limestone_exception to be thrown"; // Fails the test if no exception is thrown
} catch (const limestone_exception& e) {
// Verify the exception details
std::cerr << "Caught exception: " << e.what() << std::endl;
EXPECT_TRUE(std::string(e.what()).rfind("I/O Error (No such file or directory): Failed to rename epoch_file from /tmp/rotation_task_test/data_location/epoch", 0) == 0);
EXPECT_EQ(e.error_code(), ENOENT);
} catch (const std::exception& e) {
// Handle non-limestone_exception std::exception types
std::cerr << "Caught exception: " << e.what() << std::endl;
FAIL() << "Expected limestone_exception but caught a different std: " << e.what();
} catch (...) {
// Handle unknown exception types
FAIL() << "Expected limestone_exception but caught an unknown exception type.";
}
}

} // namespace limestone::testing
57 changes: 55 additions & 2 deletions test/limestone/log/rotate_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
#include "internal.h"

#include "test_root.h"
#include "limestone/api/limestone_exception.h"

using limestone::api::limestone_exception;
#include "rotation_task.h"

#define LOGFORMAT_VER 2

Expand Down Expand Up @@ -59,13 +63,24 @@ class rotate_test : public ::testing::Test {
// Repeatally call switch_epoch until backup is completed in another thread
std::thread switch_epoch_thread([&]() {
while (!backup_completed.load()) {
datastore_->switch_epoch(epoch_value++);
datastore_->switch_epoch(epoch_value++);
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
});

// Begin backup and wait for completion in main thread
std::unique_ptr<limestone::api::backup_detail> bd = datastore_->begin_backup(type);
std::unique_ptr<limestone::api::backup_detail> bd;

try {
// Begin backup and wait for completion in main thread
bd = datastore_->begin_backup(type);
} catch (const std::exception& e) {
// Ensure the thread is joined even in case of an exception
std::cerr << "Exception during backup: " << e.what() << std::endl;
backup_completed.store(true);
switch_epoch_thread.join();
throw;
}

// Set flag to notify backup completion
backup_completed.store(true);
Expand All @@ -80,6 +95,44 @@ class rotate_test : public ::testing::Test {
extern void create_file(const boost::filesystem::path& path, std::string_view content);
extern std::string data_manifest(int persistent_format_version = 1);


TEST_F(rotate_test, rotate_fails_with_io_error) {
using namespace limestone::api;

log_channel& channel = datastore_->create_channel(boost::filesystem::path(location));
log_channel& unused_channel = datastore_->create_channel(boost::filesystem::path(location));
datastore_->switch_epoch(42);
channel.begin_session();
channel.add_entry(42, "k1", "v1", {100, 4});
channel.end_session();
datastore_->switch_epoch(43);

// Force an exception to be thrown by removing the directory
if (system("rm -rf /tmp/rotate_test") != 0) {
std::cerr << "Cannot remove directory" << std::endl;
}

// Check that an exception is thrown and verify its details
try {
run_backup_with_epoch_switch(backup_type::standard, 44);
FAIL() << "Expected limestone_exception to be thrown"; // Fails the test if no exception is thrown
} catch (const limestone_exception& e) {
// Verify the exception details
std::cerr << "Caught exception: " << e.what() << std::endl;
EXPECT_TRUE(std::string(e.what()).rfind("I/O Error (No such file or directory): Failed to rename epoch_file from /tmp", 0) == 0);
EXPECT_EQ(e.error_code(), ENOENT);
} catch (const std::exception& e) {
// Handle non-limestone_exception std::exception types
std::cerr << "Caught exception: " << e.what() << std::endl;
FAIL() << "Expected limestone_exception but caught a different std: " << e.what();
} catch (...) {
// Handle unknown exception types
FAIL() << "Expected limestone_exception but caught an unknown exception type.";
}

}


TEST_F(rotate_test, log_is_rotated) { // NOLINT
using namespace limestone::api;

Expand Down

0 comments on commit 44854c6

Please sign in to comment.