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