Skip to content

Commit d98b69f

Browse files
rahuls-dbIsaac
andcommitted
Drop the write-only fallbackKernelBackend field
After close() switched to awaiting the in-flight connect, the fallbackKernelBackend field became write-only: assigned in the connect IIFE and cleared in close(), but read nowhere. It also left a confusing stale write — a close() racing an in-flight connect nulled the field before the IIFE re-assigned it onto an already-closed backend. Remove the field; the fallbackKernelBackendConnect promise (whose resolved value is the backend) is the single source of truth. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
1 parent cc83288 commit d98b69f

1 file changed

Lines changed: 10 additions & 13 deletions

File tree

lib/thrift-backend/ThriftBackend.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ export default class ThriftBackend implements IBackend {
3737

3838
private connectionOptions?: ConnectionOptions;
3939

40-
// A single KernelBackend reused for every Reyden (KP001) fallback session on this
41-
// connection. connect() installs a process-global log-bridge listener, so it is created
42-
// once (connectionOptions are fixed after connect) and released in close() — rather than
43-
// constructing one per openSession and leaking a listener each time.
44-
private fallbackKernelBackend?: KernelBackend;
45-
40+
// The memoized connect for a single KernelBackend, reused across every Reyden (KP001)
41+
// fallback session on this connection. connect() installs a process-global log-bridge
42+
// listener, so the backend is created once (connectionOptions are fixed after connect)
43+
// and released in close() — rather than constructing one per openSession and leaking a
44+
// listener each time. This promise is the single source of truth for the fallback
45+
// backend; its resolved value is the backend.
4646
private fallbackKernelBackendConnect?: Promise<KernelBackend>;
4747

4848
constructor({ context, onConnectionEvent }: ThriftBackendOptions) {
@@ -210,7 +210,6 @@ export default class ThriftBackend implements IBackend {
210210
this.fallbackKernelBackendConnect = (async () => {
211211
const kernelBackend = this.createKernelBackend();
212212
await kernelBackend.connect(connectionOptions);
213-
this.fallbackKernelBackend = kernelBackend;
214213
return kernelBackend;
215214
})().catch((error) => {
216215
this.fallbackKernelBackendConnect = undefined;
@@ -230,13 +229,11 @@ export default class ThriftBackend implements IBackend {
230229
// DBSQLClient owns the rest of the connection lifecycle and clears its own state
231230
// (connectionProvider, authProvider, thrift client) after this returns.
232231
//
233-
// Await the in-flight connect attempt rather than only the resolved backend:
234-
// getFallbackKernelBackend assigns this.fallbackKernelBackend only after connect()
235-
// resolves, so a close() racing an unresolved fallback connect would otherwise skip
236-
// it and leak the listener the pending connect is about to install. Clear both fields
237-
// first so the state is consistent even if the awaited close() throws.
232+
// Await the in-flight connect rather than a resolved-backend field: the connect
233+
// installs the listener only once it resolves, so a close() racing an unresolved
234+
// fallback connect must still wait for it and release the backend it produces.
235+
// Clear the field first so the state is consistent even if the awaited close() throws.
238236
const pendingConnect = this.fallbackKernelBackendConnect;
239-
this.fallbackKernelBackend = undefined;
240237
this.fallbackKernelBackendConnect = undefined;
241238
if (pendingConnect) {
242239
const kernelBackend = await pendingConnect.catch(() => undefined);

0 commit comments

Comments
 (0)