diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts index ae9eac969..e1a7979f4 100644 --- a/packages/core/src/internal/transaction-executor.ts +++ b/packages/core/src/internal/transaction-executor.ts @@ -32,6 +32,21 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout +type ClearTimeout = (timeoutId: Timeout) => void + +interface Dependencies { + setTimeout: SetTimeout + clearTimeout: ClearTimeout +} + +function setTimeoutWrapper (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { + return setTimeout(callback, ms, ...args) +} + +function clearTimeoutWrapper (timeoutId: Timeout): void { + return clearTimeout(timeoutId) +} export class TransactionExecutor { private readonly _maxRetryTimeMs: number @@ -39,13 +54,19 @@ export class TransactionExecutor { private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] + private readonly _setTimeout: SetTimeout + private readonly _clearTimeout: ClearTimeout public pipelineBegin: boolean constructor ( maxRetryTimeMs?: number | null, initialRetryDelayMs?: number, multiplier?: number, - jitterFactor?: number + jitterFactor?: number, + dependencies: Dependencies = { + setTimeout: setTimeoutWrapper, + clearTimeout: clearTimeoutWrapper + } ) { this._maxRetryTimeMs = _valueOrDefault( maxRetryTimeMs, @@ -64,6 +85,9 @@ export class TransactionExecutor { DEFAULT_RETRY_DELAY_JITTER_FACTOR ) + this._setTimeout = dependencies.setTimeout + this._clearTimeout = dependencies.clearTimeout + this._inFlightTimeoutIds = [] this.pipelineBegin = false @@ -99,7 +123,7 @@ export class TransactionExecutor { close (): void { // cancel all existing timeouts to prevent further retries - this._inFlightTimeoutIds.forEach(timeoutId => clearTimeout(timeoutId)) + this._inFlightTimeoutIds.forEach(timeoutId => this._clearTimeout(timeoutId)) this._inFlightTimeoutIds = [] } @@ -119,7 +143,7 @@ export class TransactionExecutor { return new Promise((resolve, reject) => { const nextRetryTime = this._computeDelayWithJitter(retryDelayMs) - const timeoutId = setTimeout(() => { + const timeoutId = this._setTimeout(() => { // filter out this timeoutId when time has come and function is being executed this._inFlightTimeoutIds = this._inFlightTimeoutIds.filter( id => id !== timeoutId diff --git a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts index e95ce3a9a..27b055ba3 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/transaction-executor.ts @@ -32,6 +32,21 @@ type TransactionWork = (tx: Tx) => T | Promise type Resolve = (value: T | PromiseLike) => void type Reject = (value: any) => void type Timeout = ReturnType +type SetTimeout = (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]) => Timeout +type ClearTimeout = (timeoutId: Timeout) => void + +interface Dependencies { + setTimeout: SetTimeout + clearTimeout: ClearTimeout +} + +function setTimeoutWrapper (callback: (...args: unknown[]) => void, ms: number | undefined, ...args: unknown[]): Timeout { + return setTimeout(callback, ms, ...args) +} + +function clearTimeoutWrapper (timeoutId: Timeout): void { + return clearTimeout(timeoutId) +} export class TransactionExecutor { private readonly _maxRetryTimeMs: number @@ -39,13 +54,19 @@ export class TransactionExecutor { private readonly _multiplier: number private readonly _jitterFactor: number private _inFlightTimeoutIds: Timeout[] + private readonly _setTimeout: SetTimeout + private readonly _clearTimeout: ClearTimeout public pipelineBegin: boolean constructor ( maxRetryTimeMs?: number | null, initialRetryDelayMs?: number, multiplier?: number, - jitterFactor?: number + jitterFactor?: number, + dependencies: Dependencies = { + setTimeout: setTimeoutWrapper, + clearTimeout: clearTimeoutWrapper + } ) { this._maxRetryTimeMs = _valueOrDefault( maxRetryTimeMs, @@ -64,6 +85,9 @@ export class TransactionExecutor { DEFAULT_RETRY_DELAY_JITTER_FACTOR ) + this._setTimeout = dependencies.setTimeout + this._clearTimeout = dependencies.clearTimeout + this._inFlightTimeoutIds = [] this.pipelineBegin = false @@ -99,7 +123,7 @@ export class TransactionExecutor { close (): void { // cancel all existing timeouts to prevent further retries - this._inFlightTimeoutIds.forEach(timeoutId => clearTimeout(timeoutId)) + this._inFlightTimeoutIds.forEach(timeoutId => this._clearTimeout(timeoutId)) this._inFlightTimeoutIds = [] } @@ -119,7 +143,7 @@ export class TransactionExecutor { return new Promise((resolve, reject) => { const nextRetryTime = this._computeDelayWithJitter(retryDelayMs) - const timeoutId = setTimeout(() => { + const timeoutId = this._setTimeout(() => { // filter out this timeoutId when time has come and function is being executed this._inFlightTimeoutIds = this._inFlightTimeoutIds.filter( id => id !== timeoutId diff --git a/packages/neo4j-driver/test/internal/timers-util.js b/packages/neo4j-driver/test/internal/timers-util.js index 01ad01742..2d1ee8708 100644 --- a/packages/neo4j-driver/test/internal/timers-util.js +++ b/packages/neo4j-driver/test/internal/timers-util.js @@ -15,48 +15,39 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. +*/ + +/** + * This is a lighter mock which only creates mocked functions to work + * as timeouts. */ -class SetTimeoutMock { +class TimeoutsMock { constructor () { - this._clearState() + this.clearState() + // bind it to be used as standalone functions + this.setTimeout = this.setTimeout.bind(this) + this.clearTimeout = this.clearTimeout.bind(this) } - install () { - this._originalSetTimeout = global.setTimeout - global.setTimeout = (code, delay) => { - if (!this._paused) { - code() - this.invocationDelays.push(delay) - } - return this._timeoutIdCounter++ - } - - this._originalClearTimeout = global.clearTimeout - global.clearTimeout = id => { - this.clearedTimeouts.push(id) + setTimeout (code, delay) { + const timeoutId = this._timeoutIdCounter++ + this.invocationDelays.push(delay) + if (!this._timeoutCallbacksDisabled) { + code() } - - return this - } - - pause () { - this._paused = true + return timeoutId } - uninstall () { - global.setTimeout = this._originalSetTimeout - global.clearTimeout = this._originalClearTimeout - this._clearState() + clearTimeout (id) { + this.clearedTimeouts.push(id) } - setTimeoutOriginal (code, delay) { - return this._originalSetTimeout.call(null, code, delay) + disableTimeoutCallbacks () { + this._timeoutCallbacksDisabled = true } - _clearState () { - this._originalSetTimeout = null - this._originalClearTimeout = null - this._paused = false + clearState () { + this._timeoutCallbacksDisabled = false this._timeoutIdCounter = 0 this.invocationDelays = [] @@ -64,4 +55,6 @@ class SetTimeoutMock { } } -export const setTimeoutMock = new SetTimeoutMock() +export { + TimeoutsMock +} diff --git a/packages/neo4j-driver/test/internal/transaction-executor.test.js b/packages/neo4j-driver/test/internal/transaction-executor.test.js index e0c10ba48..60763e8ae 100644 --- a/packages/neo4j-driver/test/internal/transaction-executor.test.js +++ b/packages/neo4j-driver/test/internal/transaction-executor.test.js @@ -18,7 +18,7 @@ */ import { newError, error as err, internal } from 'neo4j-driver-core' -import { setTimeoutMock } from './timers-util' +import { TimeoutsMock } from './timers-util' import lolex from 'lolex' const { @@ -89,36 +89,31 @@ describe('#unit TransactionExecutor', () => { }, 60000) it('should cancel in-flight timeouts when closed', async () => { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - // do not execute setTimeout callbacks - fakeSetTimeout.pause() + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + // do not execute setTimeout callbacks + fakeSetTimeout.disableTimeoutCallbacks() - executor.execute(transactionCreator([SERVICE_UNAVAILABLE]), () => - Promise.resolve(42) - ) - executor.execute(transactionCreator([TRANSIENT_ERROR_1]), () => - Promise.resolve(4242) - ) - executor.execute(transactionCreator([SESSION_EXPIRED]), () => - Promise.resolve(424242) - ) + executor.execute(transactionCreator([SERVICE_UNAVAILABLE]), () => + Promise.resolve(42) + ) + executor.execute(transactionCreator([TRANSIENT_ERROR_1]), () => + Promise.resolve(4242) + ) + executor.execute(transactionCreator([SESSION_EXPIRED]), () => + Promise.resolve(424242) + ) - await new Promise((resolve, reject) => { - fakeSetTimeout.setTimeoutOriginal(() => { - try { - executor.close() - expect(fakeSetTimeout.clearedTimeouts.length).toEqual(3) - resolve() - } catch (error) { - reject(error) - } - }, 1000) - }) - } finally { - fakeSetTimeout.uninstall() - } + await new Promise((resolve, reject) => { + setTimeout(() => { + try { + executor.close() + expect(fakeSetTimeout.clearedTimeouts.length).toEqual(3) + resolve() + } catch (error) { + reject(error) + } + }, 1000) + }) }, 60000) }) @@ -320,178 +315,168 @@ describe('#unit TransactionExecutor', () => { }) async function testRetryWhenTransactionCreatorFails (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const transactionCreator = throwingTransactionCreator( - errorCodes, - new FakeTransaction() - ) - const usedTransactions = [] - - const result = await executor.execute(transactionCreator, tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return Promise.resolve(42) - }) - - expect(usedTransactions.length).toEqual(1) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const transactionCreator = throwingTransactionCreator( + errorCodes, + new FakeTransaction() + ) + const usedTransactions = [] + + const result = await executor.execute(transactionCreator, tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return Promise.resolve(42) + }) + + expect(usedTransactions.length).toEqual(1) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionBeginFails (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const transactionCreator = throwingTransactionCreatorOnBegin( - errorCodes, - new FakeTransaction() - ) - const usedTransactions = [] - const beginTransactions = [] - - const result = await executor.execute(transactionCreator, async tx => { - expect(tx).toBeDefined() - beginTransactions.push(tx) + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const transactionCreator = throwingTransactionCreatorOnBegin( + errorCodes, + new FakeTransaction() + ) + const usedTransactions = [] + const beginTransactions = [] + + const result = await executor.execute(transactionCreator, async tx => { + expect(tx).toBeDefined() + beginTransactions.push(tx) - if (pipelineBegin) { - // forcing await for tx since pipeline doesn't wait for begin return - await tx - } + if (pipelineBegin) { + // forcing await for tx since pipeline doesn't wait for begin return + await tx + } - usedTransactions.push(tx) - return Promise.resolve(42) - }) - - expect(beginTransactions.length).toEqual(pipelineBegin ? errorCodes.length + 1 : 1) - expect(usedTransactions.length).toEqual(1) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + usedTransactions.push(tx) + return Promise.resolve(42) + }) + + expect(beginTransactions.length).toEqual(pipelineBegin ? errorCodes.length + 1 : 1) + expect(usedTransactions.length).toEqual(1) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionWorkReturnsRejectedPromise ( errorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = transactionWork(errorCodes, 42) - - const result = await executor.execute(transactionCreator(), tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - }) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = transactionWork(errorCodes, 42) + + const result = await executor.execute(transactionCreator(), tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return realWork() + }) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionCommitReturnsRejectedPromise ( errorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = () => Promise.resolve(4242) - - const result = await executor.execute( - transactionCreator(errorCodes), - tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - } - ) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(4242) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } - } - - async function testRetryWhenTransactionWorkThrows (errorCodes) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = throwingTransactionWork(errorCodes, 42) - - const result = await executor.execute(transactionCreator(), async tx => { + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = () => Promise.resolve(4242) + + const result = await executor.execute( + transactionCreator(errorCodes), + tx => { expect(tx).toBeDefined() usedTransactions.push(tx) - if (pipelineBegin) { - await tx - } return realWork() - }) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(errorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(42) - verifyRetryDelays(fakeSetTimeout, errorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + } + ) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(4242) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) + } + + async function testRetryWhenTransactionWorkThrows (errorCodes) { + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = throwingTransactionWork(errorCodes, 42) + + const result = await executor.execute(transactionCreator(), async tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + if (pipelineBegin) { + await tx + } + return realWork() + }) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(errorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(42) + verifyRetryDelays(fakeSetTimeout, errorCodes.length) } async function testRetryWhenTransactionWorkThrowsAndRollbackFails ( txWorkErrorCodes, rollbackErrorCodes ) { - const fakeSetTimeout = setTimeoutMock.install() - try { - const executor = new TransactionExecutor() - executor.pipelineBegin = pipelineBegin - const usedTransactions = [] - const realWork = throwingTransactionWork(txWorkErrorCodes, 424242) - - const result = await executor.execute( - transactionCreator([], rollbackErrorCodes), - tx => { - expect(tx).toBeDefined() - usedTransactions.push(tx) - return realWork() - } - ) - - // work should have failed 'failures.length' times and succeeded 1 time - expect(usedTransactions.length).toEqual(txWorkErrorCodes.length + 1) - expectAllTransactionsToBeClosed(usedTransactions) - expect(result).toEqual(424242) - verifyRetryDelays(fakeSetTimeout, txWorkErrorCodes.length) - } finally { - fakeSetTimeout.uninstall() - } + const { fakeSetTimeout, executor } = createTransactionExecutorWithFakeTimeout() + executor.pipelineBegin = pipelineBegin + const usedTransactions = [] + const realWork = throwingTransactionWork(txWorkErrorCodes, 424242) + + const result = await executor.execute( + transactionCreator([], rollbackErrorCodes), + tx => { + expect(tx).toBeDefined() + usedTransactions.push(tx) + return realWork() + } + ) + + // work should have failed 'failures.length' times and succeeded 1 time + expect(usedTransactions.length).toEqual(txWorkErrorCodes.length + 1) + expectAllTransactionsToBeClosed(usedTransactions) + expect(result).toEqual(424242) + verifyRetryDelays(fakeSetTimeout, txWorkErrorCodes.length) } }) }) +function createTransactionExecutorWithFakeTimeout (...args) { + const fakeSetTimeout = new TimeoutsMock() + const dependencies = { + setTimeout: fakeSetTimeout.setTimeout, + clearTimeout: fakeSetTimeout.clearTimeout + } + + if (typeof args[4] === 'object' || args[4] === undefined) { + args[4] = { ...dependencies, ...args[4] } + } else { + throw new TypeError( + `Expected object or undefined as args[4] but got ${typeof args[4]}`) + } + + return { + executor: new TransactionExecutor(...args), + fakeSetTimeout + } +} + async function testNoRetryOnUnknownError ( errorCodes, expectedWorkInvocationCount