From 46cc5712d5d48865512d2bc0319f3150417386e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 17 Jun 2024 12:39:26 +0200 Subject: [PATCH 1/6] Introduce `AbortSignal` to `Driver.executeQuery` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **⚠️ This API is released as preview.** This configuration property enables the cancellation of ongoing queries which was previous not possible when using `Driver.executeQuery`. The cancellation is done by closing the session used internally by the driver when execute the query. This means the cancellation is not safe and transaction might be commit apart of the execution be aborted. The cancellation might also triggers errors on the `Driver.executeQuery` execution. Example: ```javascript const abortController = new AbortController() // Some event which cancels the query process.on('SIGINT', () => abortController.abort() ) const result = await driver.executeQuery(query, params, { database: 'neo4j', signal: abortController.signal }) ``` **⚠️ This API is released as preview.** --- packages/core/src/driver.ts | 21 +++- packages/core/src/internal/query-executor.ts | 26 ++++ packages/core/test/driver.test.ts | 5 +- .../core/test/internal/query-executor.test.ts | 114 +++++++++++++++++- packages/neo4j-driver-deno/lib/core/driver.ts | 21 +++- .../lib/core/internal/query-executor.ts | 28 +++++ 6 files changed, 211 insertions(+), 4 deletions(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index e045204cb..24ef54bb4 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -358,6 +358,7 @@ class QueryConfig { resultTransformer?: ResultTransformer transactionConfig?: TransactionConfig auth?: AuthToken + signal?: AbortSignal /** * @constructor @@ -429,6 +430,23 @@ class QueryConfig { * @see {@link driver} */ this.auth = undefined + + /** + * The {@link AbortSignal} for aborting query execution. + * + * When aborted, the signal triggers the result consumption cancelation and + * transactions are reset. However, due to race conditions, + * there is no warranty the transaction will be rolled back. + * Equivalent to {@link Session.close} + * + * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener. + * + * @since 5.22.0 + * @type {AbortSignal|undefined} + * @experimental + * @see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener + */ + this.signal = undefined } } @@ -595,7 +613,8 @@ class Driver { database: config.database, impersonatedUser: config.impersonatedUser, transactionConfig: config.transactionConfig, - auth: config.auth + auth: config.auth, + signal: config.signal }, query, parameters) } diff --git a/packages/core/src/internal/query-executor.ts b/packages/core/src/internal/query-executor.ts index aaf7f459a..78c9bf6c2 100644 --- a/packages/core/src/internal/query-executor.ts +++ b/packages/core/src/internal/query-executor.ts @@ -33,6 +33,7 @@ interface ExecutionConfig { bookmarkManager?: BookmarkManager transactionConfig?: TransactionConfig auth?: AuthToken + signal?: AbortSignal resultTransformer: (result: Result) => Promise } @@ -49,6 +50,9 @@ export default class QueryExecutor { auth: config.auth }) + // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. + const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) + // @ts-expect-error The method is private for external users session._configureTransactionExecutor(true, TELEMETRY_APIS.EXECUTE_QUERY) @@ -62,7 +66,29 @@ export default class QueryExecutor { return await config.resultTransformer(result) }, config.transactionConfig) } finally { + listenerHandle.uninstall() await session.close() } } } + +type Listener = (event: unknown) => unknown + +interface EventTarget { + addEventListener?: (type: string, listener: Listener) => unknown + removeEventListener?: (type: string, listener: Listener) => unknown +} + +function installEventListenerWhenPossible (target: EventTarget | undefined, event: string, listener: () => unknown): { uninstall: () => void } { + if (typeof target?.addEventListener === 'function') { + target.addEventListener(event, listener) + } + + return { + uninstall: () => { + if (typeof target?.removeEventListener === 'function') { + target.removeEventListener(event, listener) + } + } + } +} diff --git a/packages/core/test/driver.test.ts b/packages/core/test/driver.test.ts index f4e810efb..d051f32c3 100644 --- a/packages/core/test/driver.test.ts +++ b/packages/core/test/driver.test.ts @@ -473,6 +473,8 @@ describe('Driver', () => { key: 'value' } } + const aAbortController = new AbortController() + async function aTransformer (result: Result): Promise { const summary = await result.summary() return summary.database.name ?? 'no-db-set' @@ -488,7 +490,8 @@ describe('Driver', () => { ['config.bookmarkManager=null', 'q', {}, { bookmarkManager: null }, extendsDefaultWith({ bookmarkManager: undefined })], ['config.bookmarkManager set to non-null/empty', 'q', {}, { bookmarkManager: theBookmarkManager }, extendsDefaultWith({ bookmarkManager: theBookmarkManager })], ['config.resultTransformer set', 'q', {}, { resultTransformer: aTransformer }, extendsDefaultWith({ resultTransformer: aTransformer })], - ['config.transactionConfig set', 'q', {}, { transactionConfig: aTransactionConfig }, extendsDefaultWith({ transactionConfig: aTransactionConfig })] + ['config.transactionConfig set', 'q', {}, { transactionConfig: aTransactionConfig }, extendsDefaultWith({ transactionConfig: aTransactionConfig })], + ['config.signal set', 'q', {}, { signal: aAbortController.signal }, extendsDefaultWith({ signal: aAbortController.signal })] ])('should handle the params for %s', async (_, query, params, config, buildExpectedConfig) => { const spiedExecute = jest.spyOn(queryExecutor, 'execute') diff --git a/packages/core/test/internal/query-executor.test.ts b/packages/core/test/internal/query-executor.test.ts index 4bf67cb89..a2cbb4770 100644 --- a/packages/core/test/internal/query-executor.test.ts +++ b/packages/core/test/internal/query-executor.test.ts @@ -33,6 +33,8 @@ describe('QueryExecutor', () => { } } + const aAbortController = new AbortController() + it.each([ ['bookmarkManager set', { bookmarkManager: aBookmarkManager }, { bookmarkManager: aBookmarkManager }], ['bookmarkManager undefined', { bookmarkManager: undefined }, { bookmarkManager: undefined }], @@ -41,7 +43,10 @@ describe('QueryExecutor', () => { ['impersonatedUser set', { impersonatedUser: 'anUser' }, { impersonatedUser: 'anUser' }], ['impersonatedUser undefined', { impersonatedUser: undefined }, { impersonatedUser: undefined }], ['auth set', { auth: { scheme: 'none', credentials: '' } }, { auth: { scheme: 'none', credentials: '' } }], - ['auth undefined', { auth: undefined }, { auth: undefined }] + ['auth undefined', { auth: undefined }, { auth: undefined }], + ['signal set', { signal: aAbortController.signal }, { }], + ['signal set signal', { signal: {} as unknown as AbortSignal }, { }], + ['signal undefined', { signal: undefined }, { }] ])('should redirect % to the session creation', async (_, executorConfig, expectConfig) => { const { queryExecutor, createSession } = createExecutor() @@ -208,6 +213,56 @@ describe('QueryExecutor', () => { expect(errorGot).toBe(closeError) } }) + + whenAbortSignalIsEventTarget(() => { + it('should configure listener and remove at end', async () => { + const { queryExecutor, sessionsCreated } = createExecutor() + const controller = new AbortController() + const signal = controller.signal + // @ts-expect-error + const addListenerSpy = jest.spyOn(signal, 'addEventListener') + // @ts-expect-error + const removerListenerSpy = jest.spyOn(signal, 'removeEventListener') + + const promise = queryExecutor.execute({ ...baseConfig, signal }, 'query') + + expect(addListenerSpy).toHaveBeenCalled() + expect(removerListenerSpy).not.toHaveBeenCalled() + + await promise + + expect(removerListenerSpy).toHaveBeenCalled() + + // Default expectations + expect(sessionsCreated.length).toBe(1) + const [{ spyOnExecuteRead }] = sessionsCreated + expect(spyOnExecuteRead).toHaveBeenCalled() + }) + + it('should close session when abort', async () => { + const { queryExecutor, sessionsCreated } = createExecutor() + const controller = new AbortController() + const signal = controller.signal + // @ts-expect-error + const removerListenerSpy = jest.spyOn(signal, 'removeEventListener') + + const promise = queryExecutor.execute({ ...baseConfig, signal }, 'query') + + controller.abort() + + // Expect to close session + expect(sessionsCreated[0].session.close).toHaveBeenCalled() + + await promise + + expect(removerListenerSpy).toHaveBeenCalled() + + // Default expectations + expect(sessionsCreated.length).toBe(1) + const [{ spyOnExecuteRead }] = sessionsCreated + expect(spyOnExecuteRead).toHaveBeenCalled() + }) + }) }) describe('when routing="WRITE"', () => { @@ -364,6 +419,56 @@ describe('QueryExecutor', () => { expect(errorGot).toBe(closeError) } }) + + whenAbortSignalIsEventTarget(() => { + it('should configure listener and remove at end', async () => { + const { queryExecutor, sessionsCreated } = createExecutor() + const controller = new AbortController() + const signal = controller.signal + // @ts-expect-error + const addListenerSpy = jest.spyOn(signal, 'addEventListener') + // @ts-expect-error + const removerListenerSpy = jest.spyOn(signal, 'removeEventListener') + + const promise = queryExecutor.execute({ ...baseConfig, signal }, 'query') + + expect(addListenerSpy).toHaveBeenCalled() + expect(removerListenerSpy).not.toHaveBeenCalled() + + await promise + + expect(removerListenerSpy).toHaveBeenCalled() + + // Default expectations + expect(sessionsCreated.length).toBe(1) + const [{ spyOnExecuteWrite }] = sessionsCreated + expect(spyOnExecuteWrite).toHaveBeenCalled() + }) + + it('should close session when abort', async () => { + const { queryExecutor, sessionsCreated } = createExecutor() + const controller = new AbortController() + const signal = controller.signal + // @ts-expect-error + const removerListenerSpy = jest.spyOn(signal, 'removeEventListener') + + const promise = queryExecutor.execute({ ...baseConfig, signal }, 'query') + + controller.abort() + + // Expect to close session + expect(sessionsCreated[0].session.close).toHaveBeenCalled() + + await promise + + expect(removerListenerSpy).toHaveBeenCalled() + + // Default expectations + expect(sessionsCreated.length).toBe(1) + const [{ spyOnExecuteWrite }] = sessionsCreated + expect(spyOnExecuteWrite).toHaveBeenCalled() + }) + }) }) function createExecutor ({ @@ -455,3 +560,10 @@ describe('QueryExecutor', () => { } } }) + +function whenAbortSignalIsEventTarget (fn: () => unknown): void { + // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. + if (typeof AbortSignal.prototype.addEventListener === 'function') { + describe('when abort signal is event target', fn) + } +} diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index 3f7a81b36..acbc21b6e 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -358,6 +358,7 @@ class QueryConfig { resultTransformer?: ResultTransformer transactionConfig?: TransactionConfig auth?: AuthToken + signal?: AbortSignal /** * @constructor @@ -429,6 +430,23 @@ class QueryConfig { * @see {@link driver} */ this.auth = undefined + + /** + * The {@link AbortSignal} for aborting query execution. + * + * When aborted, the signal triggers the result consumption cancelation and + * transactions are reset. However, due to race conditions, + * there is no warranty the transaction will be rolled back. + * Equivalent to {@link Session.close} + * + * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener. + * + * @since 5.22.0 + * @type {AbortSignal|undefined} + * @experimental + * @see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener + */ + this.signal = undefined } } @@ -595,7 +613,8 @@ class Driver { database: config.database, impersonatedUser: config.impersonatedUser, transactionConfig: config.transactionConfig, - auth: config.auth + auth: config.auth, + signal: config.signal }, query, parameters) } diff --git a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts index 3ee641673..5fbe092f7 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts @@ -33,6 +33,7 @@ interface ExecutionConfig { bookmarkManager?: BookmarkManager transactionConfig?: TransactionConfig auth?: AuthToken + signal?: AbortSignal resultTransformer: (result: Result) => Promise } @@ -49,6 +50,9 @@ export default class QueryExecutor { auth: config.auth }) + // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. + const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) + // @ts-expect-error The method is private for external users session._configureTransactionExecutor(true, TELEMETRY_APIS.EXECUTE_QUERY) @@ -62,7 +66,31 @@ export default class QueryExecutor { return await config.resultTransformer(result) }, config.transactionConfig) } finally { + listenerHandle.uninstall() await session.close() } } } + +interface Listener { + (event: unknown): unknown +} + +interface EventTarget { + addEventListener?: (type:string, listener: Listener) => unknown + removeEventListener?: (type:string, listener: Listener) => unknown +} + +function installEventListenerWhenPossible (target: EventTarget | undefined, event: string, listener: () => unknown): { uninstall: () => void } { + if (typeof target?.addEventListener === 'function') { + target.addEventListener(event, listener) + } + + return { + uninstall: () => { + if (typeof target?.removeEventListener === 'function') { + target.removeEventListener(event, listener) + } + } + } +} From cd37c1e60ef7b4a72c83cae81edee79fa7ffd09c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 17 Jun 2024 13:05:04 +0200 Subject: [PATCH 2/6] Sync deno --- packages/neo4j-driver-deno/lib/core/driver.ts | 10 +++++----- .../lib/core/internal/query-executor.ts | 10 ++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index acbc21b6e..cd988d1e9 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -432,15 +432,15 @@ class QueryConfig { this.auth = undefined /** - * The {@link AbortSignal} for aborting query execution. - * + * The {@link AbortSignal} for aborting query execution. + * * When aborted, the signal triggers the result consumption cancelation and * transactions are reset. However, due to race conditions, * there is no warranty the transaction will be rolled back. * Equivalent to {@link Session.close} - * - * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener. - * + * + * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener. + * * @since 5.22.0 * @type {AbortSignal|undefined} * @experimental diff --git a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts index 5fbe092f7..f4faeacc2 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts @@ -51,7 +51,7 @@ export default class QueryExecutor { }) // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. - const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) + const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) // @ts-expect-error The method is private for external users session._configureTransactionExecutor(true, TELEMETRY_APIS.EXECUTE_QUERY) @@ -72,13 +72,11 @@ export default class QueryExecutor { } } -interface Listener { - (event: unknown): unknown -} +type Listener = (event: unknown) => unknown interface EventTarget { - addEventListener?: (type:string, listener: Listener) => unknown - removeEventListener?: (type:string, listener: Listener) => unknown + addEventListener?: (type: string, listener: Listener) => unknown + removeEventListener?: (type: string, listener: Listener) => unknown } function installEventListenerWhenPossible (target: EventTarget | undefined, event: string, listener: () => unknown): { uninstall: () => void } { From 73729bb8addfee084d353fdc0cf892edb5507fe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 17 Jun 2024 13:36:33 +0200 Subject: [PATCH 3/6] Address type + linter issues --- packages/core/src/internal/query-executor.ts | 7 +++++-- .../neo4j-driver-deno/lib/core/internal/query-executor.ts | 8 ++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/core/src/internal/query-executor.ts b/packages/core/src/internal/query-executor.ts index 78c9bf6c2..53074ef3e 100644 --- a/packages/core/src/internal/query-executor.ts +++ b/packages/core/src/internal/query-executor.ts @@ -50,8 +50,11 @@ export default class QueryExecutor { auth: config.auth }) - // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. - const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) + const listenerHandle = installEventListenerWhenPossible( + // Solving linter and types definitions issue + config.signal as unknown as EventTarget, + 'abort', + async () => await session.close()) // @ts-expect-error The method is private for external users session._configureTransactionExecutor(true, TELEMETRY_APIS.EXECUTE_QUERY) diff --git a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts index f4faeacc2..93cb299da 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts @@ -50,8 +50,12 @@ export default class QueryExecutor { auth: config.auth }) - // @ts-expect-error AbortSignal doesn't implements EventTarget on this TS Config. - const listenerHandle = installEventListenerWhenPossible(config.signal, 'abort', async () => await session.close()) + + const listenerHandle = installEventListenerWhenPossible( + // Solving linter and types definitions issue + config.signal as unknown as EventTarget, + 'abort', + async () => await session.close()) // @ts-expect-error The method is private for external users session._configureTransactionExecutor(true, TELEMETRY_APIS.EXECUTE_QUERY) From 74fa41c3a5cb80dd789b2c38c242166aed2ed8ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 17 Jun 2024 14:27:21 +0200 Subject: [PATCH 4/6] sync --- .../neo4j-driver-deno/lib/core/internal/query-executor.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts index 93cb299da..d26dc9333 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/query-executor.ts @@ -50,11 +50,10 @@ export default class QueryExecutor { auth: config.auth }) - const listenerHandle = installEventListenerWhenPossible( // Solving linter and types definitions issue - config.signal as unknown as EventTarget, - 'abort', + config.signal as unknown as EventTarget, + 'abort', async () => await session.close()) // @ts-expect-error The method is private for external users From 9dda854e4e993049598fffd64093c1d4ca5c6976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Tue, 18 Jun 2024 10:13:45 +0200 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: gjmwoods <42248895+gjmwoods@users.noreply.github.com> --- packages/core/src/driver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 24ef54bb4..d08883799 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -436,7 +436,7 @@ class QueryConfig { * * When aborted, the signal triggers the result consumption cancelation and * transactions are reset. However, due to race conditions, - * there is no warranty the transaction will be rolled back. + * there is no guarantee the transaction will be rolled back. * Equivalent to {@link Session.close} * * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener. From 83d66f8ff5f509a7b4ebf9536b609c34d582fcd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Tue, 18 Jun 2024 10:15:57 +0200 Subject: [PATCH 6/6] sync deno --- packages/neo4j-driver-deno/lib/core/driver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index cd988d1e9..6331c8146 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -436,7 +436,7 @@ class QueryConfig { * * When aborted, the signal triggers the result consumption cancelation and * transactions are reset. However, due to race conditions, - * there is no warranty the transaction will be rolled back. + * there is no guarantee the transaction will be rolled back. * Equivalent to {@link Session.close} * * **Warning**: This option is only available in runtime which supports AbortSignal.addEventListener.