Skip to content

fix: DH-23437: Create the C++ client console lazily - #8398

Open
jmao-denver wants to merge 1 commit into
deephaven:mainfrom
jmao-denver:jmao-DH-23437-lazy-console
Open

fix: DH-23437: Create the C++ client console lazily#8398
jmao-denver wants to merge 1 commit into
deephaven:mainfrom
jmao-denver:jmao-DH-23437-lazy-console

Conversation

@jmao-denver

Copy link
Copy Markdown
Contributor

Fixes DH-23437.

Problem

Client::Connect issued a ConsoleService.StartConsole RPC whenever the session type was non-empty — and ClientOptions defaults it to "python". Deployments that restrict consoles to administrators therefore rejected the connection outright, even for clients that never run a script.

This surfaced in the Deephaven Enterprise Flight SQL ODBC driver: creating a DSN and clicking Test Connection failed for a user who is only a viewer on the target Persistent Query. The driver only needs the worker's FlightClient; the console ticket it was forced to obtain was never used.

Why deferring is correct

StartConsole does not create the script session — the worker already has one (scriptSessionProvider.get() in ConsoleServiceGrpcImpl#startConsole). The RPC validates the requested language and mints a ticket naming that existing session. Nothing needs it at connect time.

Java (SessionImpl.console on first executeCode) and Python (ConsoleService.start_console from run_script) already create the console on first use. C++ was the outlier.

Change

TableHandleManagerImpl now holds the session type and starts the console on first use, in EnsureConsoleId(), guarded by its own mutex (not the one protecting subscriptions_, since console creation makes an RPC under the lock). RunScript and BindToVariable are the only two consumers of the console ticket.

All changes are in private headers, so no public API changes.

Behavior change

Console permission failures and the server's session-type check (ConsoleServiceGrpcImpl.java"session type 'x' is not supported") now surface at the first RunScript/BindToVariable instead of at Connect. The error message for a client created without a script language is unchanged, and the existing test that pins it (script_test.cc, "Script session error") passes untouched.

First RunScript/BindToVariable costs one extra round trip.

Testing

Two tests added to script_test.cc:

  • table operations succeed on a client that never runs a script, so connect no longer needs a console;
  • two scripts on one client see each other's variables, so the lazily created console is cached rather than recreated.

The C++ client only builds on Linux and Windows, so I have not built or run these locally — relying on CI.

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) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 20, 2026 19:50
@github-actions

Copy link
Copy Markdown
Contributor

No docs changes detected for fa29e97

@jmao-denver jmao-denver self-assigned this Aug 20, 2026
@jmao-denver
jmao-denver marked this pull request as draft August 20, 2026 19:52
@jmao-denver
jmao-denver requested review from cpwright and kosak August 20, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Defers C++ console creation until scripting is first used, allowing table-only clients to connect without console permissions.

Changes:

  • Lazily starts and caches the console ticket.
  • Uses lazy initialization for scripts and variable binding.
  • Adds regression tests for console-free operations and script reuse.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
script_test.cc Adds lazy-console behavior tests.
table_handle_manager_impl.cc Implements synchronized console initialization.
table_handle_impl.cc Lazily obtains the console for binding.
client_impl.cc Removes eager console startup.
table_handle_manager_impl.h Stores session type and lazy-console state.
Suppressed comments (1)

cpp-client/deephaven/tests/src/script_test.cc:68

  • This does not verify that the console ticket is reused. Each StartConsole creates a new DelegatingScriptSession, but all such wrappers delegate execution and query scope to the same underlying server ScriptSession, so t1 remains visible even if StartConsole is called before every script. Please verify the StartConsole call count or ticket identity with a test double/instrumented server instead.
  // t1 is only in scope if both scripts hit the same console.
  thm.RunScript("t2 = t1.update([\"x = ii\"])");

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


TEST_CASE("Table operations do not need a console", "[script]") {
// Never runs a script, so no console should be needed.
auto client = TableMakerForTests::CreateClient();
@jmao-denver
jmao-denver marked this pull request as ready for review August 25, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants