From 02f50d59212c7afb4f4dab3f3541a12cd226023b Mon Sep 17 00:00:00 2001 From: wudidapaopao Date: Thu, 31 Jul 2025 02:36:09 +0800 Subject: [PATCH 1/5] fix: fix open session after a previous failure --- chdb/session/state.py | 1 + programs/local/chdb.cpp | 42 ++++++++++++++---------- tests/test_open_session_after_failure.py | 36 ++++++++++++++++++++ 3 files changed, 62 insertions(+), 17 deletions(-) create mode 100644 tests/test_open_session_after_failure.py diff --git a/chdb/session/state.py b/chdb/session/state.py index be8552b7168..dc3e9fdfbb4 100644 --- a/chdb/session/state.py +++ b/chdb/session/state.py @@ -40,6 +40,7 @@ class Session: """ def __init__(self, path=None): + self._conn = None global g_session, g_session_path if g_session is not None: warnings.warn( diff --git a/programs/local/chdb.cpp b/programs/local/chdb.cpp index 3a36e4da2bf..cc1a00ce2a7 100644 --- a/programs/local/chdb.cpp +++ b/programs/local/chdb.cpp @@ -18,6 +18,23 @@ static std::mutex CHDB_MUTEX; chdb_conn * global_conn_ptr = nullptr; std::string global_db_path; +static void cleanUpLocalServer(DB::LocalServer *& server) +{ + try + { + if (server) + { + server->chdbCleanup(); + delete server; + server = nullptr; + } + } + catch (...) + { + LOG_ERROR(&Poco::Logger::get("LocalServer"), "Error during server cleanup"); + } +} + static DB::LocalServer * bgClickHouseLocal(int argc, char ** argv) { DB::LocalServer * app = nullptr; @@ -31,30 +48,29 @@ static DB::LocalServer * bgClickHouseLocal(int argc, char ** argv) { auto err_msg = app->getErrorMsg(); LOG_ERROR(&app->logger(), "Error running bgClickHouseLocal: {}", err_msg); - delete app; - app = nullptr; + cleanUpLocalServer(app); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Error running bgClickHouseLocal: {}", err_msg); } return app; } catch (const DB::Exception & e) { - delete app; + cleanUpLocalServer(app); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", DB::getExceptionMessage(e, false)); } catch (const Poco::Exception & e) { - delete app; + cleanUpLocalServer(app); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", e.displayText()); } catch (const std::exception & e) { - delete app; + cleanUpLocalServer(app); throw std::domain_error(e.what()); } catch (...) { - delete app; + cleanUpLocalServer(app); throw std::domain_error(DB::getCurrentExceptionMessage(true)); } } @@ -545,9 +561,10 @@ chdb_conn ** connect_chdb(int argc, char ** argv) [&]() { auto * queue = static_cast(conn->queue); + DB::LocalServer * server = nullptr; try { - DB::LocalServer * server = bgClickHouseLocal(argc, argv); + server = bgClickHouseLocal(argc, argv); conn->server = server; conn->connected = true; @@ -570,16 +587,7 @@ chdb_conn ** connect_chdb(int argc, char ** argv) if (queue->shutdown) { - try - { - server->chdbCleanup(); - delete server; - } - catch (...) - { - // Log error but continue shutdown - LOG_ERROR(&Poco::Logger::get("LocalServer"), "Error during server cleanup"); - } + cleanUpLocalServer(server); queue->cleanup_done = true; queue->query_cv.notify_all(); break; diff --git a/tests/test_open_session_after_failure.py b/tests/test_open_session_after_failure.py new file mode 100644 index 00000000000..4935585b668 --- /dev/null +++ b/tests/test_open_session_after_failure.py @@ -0,0 +1,36 @@ +#!python3 + +import unittest +import shutil +from chdb import session + + +test_dir1 = ".test_open_session_after_failure" +test_dir2 = "/usr/bin" + + +class TestStateful(unittest.TestCase): + def setUp(self) -> None: + shutil.rmtree(test_dir1, ignore_errors=True) + return super().setUp() + + def tearDown(self) -> None: + shutil.rmtree(test_dir1, ignore_errors=True) + return super().tearDown() + + def test_path(self): + # Test that creating session with invalid path (read-only directory) raises exception + with self.assertRaises(Exception): + sess = session.Session(test_dir2) + + # Test that creating session with valid path works after failure + sess = session.Session(test_dir1) + + ret = sess.query("select 'aaaaa'") + self.assertEqual(str(ret), "\"aaaaa\"\n") + + sess.close() + + +if __name__ == '__main__': + unittest.main() From f2512a08dc56b6a42886ffd65e80351de7a7589a Mon Sep 17 00:00:00 2001 From: wudidapaopao Date: Fri, 1 Aug 2025 13:46:00 +0800 Subject: [PATCH 2/5] chore: use smart ptr --- programs/local/chdb.cpp | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/programs/local/chdb.cpp b/programs/local/chdb.cpp index cc1a00ce2a7..5be903c70eb 100644 --- a/programs/local/chdb.cpp +++ b/programs/local/chdb.cpp @@ -18,15 +18,13 @@ static std::mutex CHDB_MUTEX; chdb_conn * global_conn_ptr = nullptr; std::string global_db_path; -static void cleanUpLocalServer(DB::LocalServer *& server) +static void cleanUpLocalServer(std::unique_ptr server) { try { if (server) { server->chdbCleanup(); - delete server; - server = nullptr; } } catch (...) @@ -35,12 +33,12 @@ static void cleanUpLocalServer(DB::LocalServer *& server) } } -static DB::LocalServer * bgClickHouseLocal(int argc, char ** argv) +static std::unique_ptr bgClickHouseLocal(int argc, char ** argv) { - DB::LocalServer * app = nullptr; + std::unique_ptr app; try { - app = new DB::LocalServer(); + app = std::make_unique(); app->setBackground(true); app->init(argc, argv); int ret = app->run(); @@ -48,29 +46,29 @@ static DB::LocalServer * bgClickHouseLocal(int argc, char ** argv) { auto err_msg = app->getErrorMsg(); LOG_ERROR(&app->logger(), "Error running bgClickHouseLocal: {}", err_msg); - cleanUpLocalServer(app); + cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Error running bgClickHouseLocal: {}", err_msg); } return app; } catch (const DB::Exception & e) { - cleanUpLocalServer(app); + cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", DB::getExceptionMessage(e, false)); } catch (const Poco::Exception & e) { - cleanUpLocalServer(app); + cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", e.displayText()); } catch (const std::exception & e) { - cleanUpLocalServer(app); + cleanUpLocalServer(std::move(app)); throw std::domain_error(e.what()); } catch (...) { - cleanUpLocalServer(app); + cleanUpLocalServer(std::move(app)); throw std::domain_error(DB::getCurrentExceptionMessage(true)); } } @@ -561,11 +559,11 @@ chdb_conn ** connect_chdb(int argc, char ** argv) [&]() { auto * queue = static_cast(conn->queue); - DB::LocalServer * server = nullptr; + std::unique_ptr server; try { server = bgClickHouseLocal(argc, argv); - conn->server = server; + conn->server = nullptr; conn->connected = true; global_conn_ptr = conn; @@ -587,7 +585,7 @@ chdb_conn ** connect_chdb(int argc, char ** argv) if (queue->shutdown) { - cleanUpLocalServer(server); + cleanUpLocalServer(std::move(server)); queue->cleanup_done = true; queue->query_cv.notify_all(); break; @@ -596,7 +594,7 @@ chdb_conn ** connect_chdb(int argc, char ** argv) } CHDB::QueryRequestBase & req = *(queue->current_query); - auto result = createQueryResult(server, req); + auto result = createQueryResult(server.get(), req); bool is_end = result.second; { From e2ce463bd7fa2eb77baac6e7f4da73d42c792b9d Mon Sep 17 00:00:00 2001 From: wudidapaopao Date: Fri, 1 Aug 2025 17:53:20 +0800 Subject: [PATCH 3/5] chore: put cleanup in destruct of LocalServer --- programs/local/LocalServer.h | 5 ----- programs/local/chdb.cpp | 21 --------------------- 2 files changed, 26 deletions(-) diff --git a/programs/local/LocalServer.h b/programs/local/LocalServer.h index ec8e2dd4349..3cc42364a3b 100644 --- a/programs/local/LocalServer.h +++ b/programs/local/LocalServer.h @@ -89,11 +89,6 @@ class LocalServer : public ClientApplicationBase, public Loggers return local_connection->getCHDBProgress().read_bytes; } - void chdbCleanup() - { - cleanup(); - } - private: void cleanStreamingQuery(); }; diff --git a/programs/local/chdb.cpp b/programs/local/chdb.cpp index 5be903c70eb..112f4e0bb95 100644 --- a/programs/local/chdb.cpp +++ b/programs/local/chdb.cpp @@ -18,21 +18,6 @@ static std::mutex CHDB_MUTEX; chdb_conn * global_conn_ptr = nullptr; std::string global_db_path; -static void cleanUpLocalServer(std::unique_ptr server) -{ - try - { - if (server) - { - server->chdbCleanup(); - } - } - catch (...) - { - LOG_ERROR(&Poco::Logger::get("LocalServer"), "Error during server cleanup"); - } -} - static std::unique_ptr bgClickHouseLocal(int argc, char ** argv) { std::unique_ptr app; @@ -46,29 +31,24 @@ static std::unique_ptr bgClickHouseLocal(int argc, char ** argv { auto err_msg = app->getErrorMsg(); LOG_ERROR(&app->logger(), "Error running bgClickHouseLocal: {}", err_msg); - cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Error running bgClickHouseLocal: {}", err_msg); } return app; } catch (const DB::Exception & e) { - cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", DB::getExceptionMessage(e, false)); } catch (const Poco::Exception & e) { - cleanUpLocalServer(std::move(app)); throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "bgClickHouseLocal {}", e.displayText()); } catch (const std::exception & e) { - cleanUpLocalServer(std::move(app)); throw std::domain_error(e.what()); } catch (...) { - cleanUpLocalServer(std::move(app)); throw std::domain_error(DB::getCurrentExceptionMessage(true)); } } @@ -585,7 +565,6 @@ chdb_conn ** connect_chdb(int argc, char ** argv) if (queue->shutdown) { - cleanUpLocalServer(std::move(server)); queue->cleanup_done = true; queue->query_cv.notify_all(); break; From 807fb8d2ee7a445d2557121e9424911e366629c7 Mon Sep 17 00:00:00 2001 From: wudidapaopao Date: Fri, 1 Aug 2025 17:54:21 +0800 Subject: [PATCH 4/5] chore: put cleanup in destruct of LocalServer --- programs/local/LocalServer.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index 105e98da52d..449f43f0d69 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -157,6 +157,7 @@ void applySettingsOverridesForLocal(ContextMutablePtr context) LocalServer::~LocalServer() { + cleanup(); resetQueryOutputVector(); } From b22a958a1af7565d2231f76acd74e31d57661f8d Mon Sep 17 00:00:00 2001 From: wudidapaopao Date: Fri, 1 Aug 2025 17:57:59 +0800 Subject: [PATCH 5/5] chore: fix chdb.cpp --- programs/local/chdb.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/local/chdb.cpp b/programs/local/chdb.cpp index 112f4e0bb95..16fe99c9a36 100644 --- a/programs/local/chdb.cpp +++ b/programs/local/chdb.cpp @@ -565,6 +565,7 @@ chdb_conn ** connect_chdb(int argc, char ** argv) if (queue->shutdown) { + server.reset(); queue->cleanup_done = true; queue->query_cv.notify_all(); break;