From fa29e974842be69208cb7ac52d71f80b6a8aa944 Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Thu, 20 Aug 2026 13:46:14 -0600 Subject: [PATCH] fix: DH-23437: Create the C++ client console lazily Client::Connect issued a ConsoleService.StartConsole RPC whenever the session type was non-empty, which ClientOptions defaults to "python". Deployments that restrict consoles to administrators therefore rejected the connection outright, even for clients that never run a script -- notably the Flight SQL ODBC driver, which only needs the worker's FlightClient. StartConsole does not create the script session; the worker already has one. It validates the requested language and mints a ticket naming that session. So the ticket can be minted on demand. TableHandleManagerImpl now holds the session type and starts the console on first use, in EnsureConsoleId(), under its own mutex. RunScript and BindToVariable are the only two consumers. Java and Python already behave this way. Behavior change: console permission failures and the server's session-type check now surface at first RunScript/BindToVariable rather than at Connect. Co-Authored-By: Claude Opus 5 (1M context) --- .../client/impl/table_handle_manager_impl.h | 14 ++++++--- .../dhclient/src/impl/client_impl.cc | 21 ++----------- .../dhclient/src/impl/table_handle_impl.cc | 8 +---- .../src/impl/table_handle_manager_impl.cc | 31 ++++++++++++++----- cpp-client/deephaven/tests/src/script_test.cc | 23 ++++++++++++++ 5 files changed, 61 insertions(+), 36 deletions(-) diff --git a/cpp-client/deephaven/dhclient/include/private/deephaven/client/impl/table_handle_manager_impl.h b/cpp-client/deephaven/dhclient/include/private/deephaven/client/impl/table_handle_manager_impl.h index 1195c7bbbf4..89dcb9c82b1 100644 --- a/cpp-client/deephaven/dhclient/include/private/deephaven/client/impl/table_handle_manager_impl.h +++ b/cpp-client/deephaven/dhclient/include/private/deephaven/client/impl/table_handle_manager_impl.h @@ -28,11 +28,11 @@ class TableHandleManagerImpl final : public std::enable_shared_from_this Create(std::optional console_id, + static std::shared_ptr Create(std::string session_type, std::shared_ptr server, std::shared_ptr executor, std::shared_ptr flight_executor); - TableHandleManagerImpl(Private, std::optional &&console_id, + TableHandleManagerImpl(Private, std::string &&session_type, std::shared_ptr &&server, std::shared_ptr &&executor, std::shared_ptr &&flight_executor); TableHandleManagerImpl(const TableHandleManagerImpl &other) = delete; @@ -68,8 +68,9 @@ class TableHandleManagerImpl final : public std::enable_shared_from_this handle); void RemoveSubscriptionHandle(const std::shared_ptr &handle); + /** Returns the console Ticket, starting the console on first call. Throws if there is no script language. */ [[nodiscard]] - const std::optional &ConsoleId() const { return consoleId_; } + const Ticket &EnsureConsoleId(); [[nodiscard]] const std::shared_ptr &Server() const { return server_; } [[nodiscard]] @@ -79,10 +80,15 @@ class TableHandleManagerImpl final : public std::enable_shared_from_this consoleId_; + // Scripting language for this session; empty means console operations throw. + const std::string sessionType_; std::shared_ptr server_; std::shared_ptr executor_; std::shared_ptr flightExecutor_; + // Guards consoleId_. Not mutex_: starting the console makes an RPC under this lock. + std::mutex consoleMutex_; + // Lazily set by EnsureConsoleId(); never reset. + std::optional consoleId_; // Protects the below for concurrent access. std::mutex mutex_; // The SubscriptionHandles for the tables we have subscribed to. We keep these at the TableHandleManagerImpl level diff --git a/cpp-client/deephaven/dhclient/src/impl/client_impl.cc b/cpp-client/deephaven/dhclient/src/impl/client_impl.cc index 917f53bcd92..ca9a2a6a870 100644 --- a/cpp-client/deephaven/dhclient/src/impl/client_impl.cc +++ b/cpp-client/deephaven/dhclient/src/impl/client_impl.cc @@ -5,14 +5,9 @@ #include #include -#include #include #include "deephaven/client/impl/table_handle_manager_impl.h" -using io::deephaven::proto::backplane::grpc::Ticket; -using io::deephaven::proto::backplane::script::grpc::StartConsoleRequest; -using io::deephaven::proto::backplane::script::grpc::StartConsoleResponse; - using deephaven::client::impl::TableHandleManagerImpl; using deephaven::client::server::Server; using deephaven::client::utility::Executor; @@ -24,20 +19,10 @@ std::shared_ptr ClientImpl::Create( std::shared_ptr executor, std::shared_ptr flight_executor, std::string session_type) { - std::optional console_ticket; - if (!session_type.empty()) { - StartConsoleRequest req; - *req.mutable_result_id() = server->NewTicket(); - *req.mutable_session_type() = std::move(session_type); - StartConsoleResponse resp; - server->SendRpc([&](grpc::ClientContext *ctx) { - return server->ConsoleStub()->StartConsole(ctx, req, &resp); - }); - console_ticket = std::move(*resp.mutable_result_id()); - } - + // No console here: TableHandleManagerImpl starts one on first use, keeping + // Client::Connect free of the ConsoleService.StartConsole RPC. auto thmi = TableHandleManagerImpl::Create( - std::move(console_ticket), + std::move(session_type), std::move(server), std::move(executor), std::move(flight_executor)); diff --git a/cpp-client/deephaven/dhclient/src/impl/table_handle_impl.cc b/cpp-client/deephaven/dhclient/src/impl/table_handle_impl.cc index 61bacebc803..9caa5a06427 100644 --- a/cpp-client/deephaven/dhclient/src/impl/table_handle_impl.cc +++ b/cpp-client/deephaven/dhclient/src/impl/table_handle_impl.cc @@ -605,14 +605,8 @@ void TableHandleImpl::Unsubscribe(const std::shared_ptr &han } void TableHandleImpl::BindToVariable(std::string variable) { - const auto &console_id = managerImpl_->ConsoleId(); - if (!console_id.has_value()) { - auto message = DEEPHAVEN_LOCATION_STR( - "Client was created without specifying a script language"); - throw std::runtime_error(message); - } BindTableToVariableRequest req; - *req.mutable_console_id() = *console_id; + *req.mutable_console_id() = managerImpl_->EnsureConsoleId(); req.set_variable_name(std::move(variable)); *req.mutable_table_id() = ticket_; diff --git a/cpp-client/deephaven/dhclient/src/impl/table_handle_manager_impl.cc b/cpp-client/deephaven/dhclient/src/impl/table_handle_manager_impl.cc index 23c0cec340a..5bf7b423787 100644 --- a/cpp-client/deephaven/dhclient/src/impl/table_handle_manager_impl.cc +++ b/cpp-client/deephaven/dhclient/src/impl/table_handle_manager_impl.cc @@ -19,24 +19,26 @@ using io::deephaven::proto::backplane::grpc::TimeTableRequest; using io::deephaven::proto::backplane::grpc::Ticket; using io::deephaven::proto::backplane::script::grpc::ExecuteCommandRequest; using io::deephaven::proto::backplane::script::grpc::ExecuteCommandResponse; +using io::deephaven::proto::backplane::script::grpc::StartConsoleRequest; +using io::deephaven::proto::backplane::script::grpc::StartConsoleResponse; namespace deephaven::client::impl { namespace { Ticket MakeScopeReference(std::string_view table_name); } // namespace -std::shared_ptr TableHandleManagerImpl::Create(std::optional console_id, +std::shared_ptr TableHandleManagerImpl::Create(std::string session_type, std::shared_ptr server, std::shared_ptr executor, std::shared_ptr flight_executor) { - return std::make_shared(Private(), std::move(console_id), + return std::make_shared(Private(), std::move(session_type), std::move(server), std::move(executor), std::move(flight_executor)); } -TableHandleManagerImpl::TableHandleManagerImpl(Private, std::optional &&console_id, +TableHandleManagerImpl::TableHandleManagerImpl(Private, std::string &&session_type, std::shared_ptr &&server, std::shared_ptr &&executor, std::shared_ptr &&flight_executor) : me_(deephaven::dhcore::utility::ObjectId("TableHandleManagerImpl", this)), - consoleId_(std::move(console_id)), + sessionType_(std::move(session_type)), server_(std::move(server)), executor_(std::move(executor)), flightExecutor_(std::move(flight_executor)) { @@ -140,13 +142,28 @@ std::shared_ptr TableHandleManagerImpl::InputTable( return TableHandleImpl::Create(shared_from_this(), std::move(resp)); } -void TableHandleManagerImpl::RunScript(std::string code) { - if (!consoleId_.has_value()) { +const Ticket &TableHandleManagerImpl::EnsureConsoleId() { + if (sessionType_.empty()) { auto message = DEEPHAVEN_LOCATION_STR("Client was created without specifying a script language"); throw std::runtime_error(message); } + std::unique_lock guard(consoleMutex_); + if (!consoleId_.has_value()) { + StartConsoleRequest req; + *req.mutable_result_id() = server_->NewTicket(); + *req.mutable_session_type() = sessionType_; + StartConsoleResponse resp; + server_->SendRpc([&](grpc::ClientContext *ctx) { + return server_->ConsoleStub()->StartConsole(ctx, req, &resp); + }); + consoleId_ = std::move(*resp.mutable_result_id()); + } + return *consoleId_; +} + +void TableHandleManagerImpl::RunScript(std::string code) { ExecuteCommandRequest req; - *req.mutable_console_id() = *consoleId_; + *req.mutable_console_id() = EnsureConsoleId(); *req.mutable_code() = std::move(code); ExecuteCommandResponse resp; server_->SendRpc([&](grpc::ClientContext *ctx) { diff --git a/cpp-client/deephaven/tests/src/script_test.cc b/cpp-client/deephaven/tests/src/script_test.cc index 8c86efb15a7..08042a77ac1 100644 --- a/cpp-client/deephaven/tests/src/script_test.cc +++ b/cpp-client/deephaven/tests/src/script_test.cc @@ -46,4 +46,27 @@ mytable = empty_table(16).update(["intData = (int)(ii - 8)", "longData = (long)( expected.AddColumn("longData", long_data); TableComparerForTests::Compare(expected, t); } + +TEST_CASE("Table operations do not need a console", "[script]") { + // Never runs a script, so no console should be needed. + auto client = TableMakerForTests::CreateClient(); + auto thm = client.GetManager(); + + auto t = thm.EmptyTable(10); + CHECK(t.NumRows() == 10); +} + +TEST_CASE("Console is reused across scripts", "[script]") { + auto client = TableMakerForTests::CreateClient(); + auto thm = client.GetManager(); + + thm.RunScript("from deephaven import empty_table\nt1 = empty_table(3)"); + auto t1 = thm.FetchTable("t1"); + CHECK(t1.NumRows() == 3); + + // t1 is only in scope if both scripts hit the same console. + thm.RunScript("t2 = t1.update([\"x = ii\"])"); + auto t2 = thm.FetchTable("t2"); + CHECK(t2.NumRows() == 3); +} } // namespace deephaven::client::tests