Skip to content

Transaction functions should retry on AuthorizationExpired error #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/src/internal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import * as logger from './logger'
import * as urlUtil from './url-util'
import * as serverAddress from './server-address'
import * as resolver from './resolver'
import * as retryStrategy from './retry-strategy'

export {
util,
Expand All @@ -44,5 +45,6 @@ export {
logger,
urlUtil,
serverAddress,
resolver
resolver,
retryStrategy
}
63 changes: 63 additions & 0 deletions core/src/internal/retry-strategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Copyright (c) "Neo4j"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* 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.
*/

import { Neo4jError, SERVICE_UNAVAILABLE, SESSION_EXPIRED } from '../error'

/**
* Verified error and returns if it could be retried or not
*
* @param _error The error
* @returns If the transaction could be retried.
*/
function canRetryOn (_error: any): boolean {
return (
_error &&
_error instanceof Neo4jError &&
_error.code &&
(_error.code === SERVICE_UNAVAILABLE ||
_error.code === SESSION_EXPIRED ||
_isAuthorizationExpired(_error) ||
_isTransientError(_error))
)
}

function _isTransientError (error: Neo4jError): boolean {
// Retries should not happen when transaction was explicitly terminated by the user.
// Termination of transaction might result in two different error codes depending on where it was
// terminated. These are really client errors but classification on the server is not entirely correct and
// they are classified as transient.

const code = error.code
if (code.indexOf('TransientError') >= 0) {
if (
code === 'Neo.TransientError.Transaction.Terminated' ||
code === 'Neo.TransientError.Transaction.LockClientStopped'
) {
return false
}
return true
}
return false
}

function _isAuthorizationExpired (error: Neo4jError): boolean {
return error.code === 'Neo.ClientError.Security.AuthorizationExpired'
}

export { canRetryOn }
41 changes: 3 additions & 38 deletions core/src/internal/transaction-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,9 @@
* limitations under the License.
*/

import {
newError,
Neo4jError,
SERVICE_UNAVAILABLE,
SESSION_EXPIRED
} from '../error'
import { newError } from '../error'
import Transaction from '../transaction'
import { canRetryOn } from './retry-strategy'

const DEFAULT_MAX_RETRY_TIME_MS = 30 * 1000 // 30 seconds
const DEFAULT_INITIAL_RETRY_DELAY_MS = 1000 // 1 seconds
Expand Down Expand Up @@ -109,10 +105,7 @@ export class TransactionExecutor {
): Promise<T> {
const elapsedTimeMs = Date.now() - retryStartTime

if (
elapsedTimeMs > this._maxRetryTimeMs ||
!TransactionExecutor._canRetryOn(error)
) {
if (elapsedTimeMs > this._maxRetryTimeMs || !canRetryOn(error)) {
return Promise.reject(error)
}

Expand Down Expand Up @@ -229,34 +222,6 @@ export class TransactionExecutor {
return Math.random() * (max - min) + min
}

static _canRetryOn(error: any): boolean {
return (error &&
error instanceof Neo4jError &&
error.code &&
(error.code === SERVICE_UNAVAILABLE ||
error.code === SESSION_EXPIRED ||
this._isTransientError(error))) as boolean
}

static _isTransientError(error: Neo4jError): boolean {
// Retries should not happen when transaction was explicitly terminated by the user.
// Termination of transaction might result in two different error codes depending on where it was
// terminated. These are really client errors but classification on the server is not entirely correct and
// they are classified as transient.

const code = error.code
if (code.indexOf('TransientError') >= 0) {
if (
code === 'Neo.TransientError.Transaction.Terminated' ||
code === 'Neo.TransientError.Transaction.LockClientStopped'
) {
return false
}
return true
}
return false
}

_verifyAfterConstruction() {
if (this._maxRetryTimeMs < 0) {
throw newError('Max retry time should be >= 0: ' + this._maxRetryTimeMs)
Expand Down
34 changes: 3 additions & 31 deletions src/internal/retry-logic-rx.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ const {
logger: {
// eslint-disable-next-line no-unused-vars
Logger
}
},
retryStrategy: { canRetryOn }
} = internal

const { SERVICE_UNAVAILABLE, SESSION_EXPIRED } = error
Expand Down Expand Up @@ -80,7 +81,7 @@ export default class RxRetryLogic {

return failedWork.pipe(
flatMap(err => {
if (!RxRetryLogic._canRetryOn(err)) {
if (!canRetryOn(err)) {
return throwError(err)
}

Expand Down Expand Up @@ -119,35 +120,6 @@ export default class RxRetryLogic {
const jitter = delay * this._delayJitter
return delay - jitter + 2 * jitter * Math.random()
}

static _canRetryOn (error) {
return (
error &&
error.code &&
(error.code === SERVICE_UNAVAILABLE ||
error.code === SESSION_EXPIRED ||
this._isTransientError(error))
)
}

static _isTransientError (error) {
// Retries should not happen when transaction was explicitly terminated by the user.
// Termination of transaction might result in two different error codes depending on where it was
// terminated. These are really client errors but classification on the server is not entirely correct and
// they are classified as transient.

const code = error.code
if (code.indexOf('TransientError') >= 0) {
if (
code === 'Neo.TransientError.Transaction.Terminated' ||
code === 'Neo.TransientError.Transaction.LockClientStopped'
) {
return false
}
return true
}
return false
}
}

function valueOrDefault (value, defaultValue) {
Expand Down
9 changes: 9 additions & 0 deletions test/internal/retry-logic-rx.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ describe('#unit-rx retrylogic', () => {
verifyRetry(newError('service unavailable', SERVICE_UNAVAILABLE))
})

it('a Neo.ClientError.Security.AuthorizationExpired error', () => {
verifyRetry(
newError(
'service unavailable',
'Neo.ClientError.Security.AuthorizationExpired'
)
)
})

function verifyRetry (error) {
scheduler.run(helpers => {
const retryLogic = new RxRetryLogic({ maxRetryTimeout: 5000 })
Expand Down
13 changes: 9 additions & 4 deletions test/internal/transaction-executor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ describe('#unit TransactionExecutor', () => {
TRANSIENT_ERROR_2,
SESSION_EXPIRED,
SERVICE_UNAVAILABLE,
TRANSIENT_ERROR_1
TRANSIENT_ERROR_1,
'Neo.ClientError.Security.AuthorizationExpired'
])
}, 30000)

Expand All @@ -203,7 +204,8 @@ describe('#unit TransactionExecutor', () => {
SERVICE_UNAVAILABLE,
TRANSIENT_ERROR_2,
TRANSIENT_ERROR_2,
SESSION_EXPIRED
SESSION_EXPIRED,
'Neo.ClientError.Security.AuthorizationExpired'
])
}, 30000)

Expand All @@ -214,7 +216,8 @@ describe('#unit TransactionExecutor', () => {
TRANSIENT_ERROR_2,
SESSION_EXPIRED,
TRANSIENT_ERROR_1,
SESSION_EXPIRED
SESSION_EXPIRED,
'Neo.ClientError.Security.AuthorizationExpired'
])
}, 30000)

Expand All @@ -230,7 +233,8 @@ describe('#unit TransactionExecutor', () => {
TRANSIENT_ERROR_1,
SESSION_EXPIRED,
SERVICE_UNAVAILABLE,
TRANSIENT_ERROR_2
TRANSIENT_ERROR_2,
'Neo.ClientError.Security.AuthorizationExpired'
])
}, 30000)

Expand All @@ -239,6 +243,7 @@ describe('#unit TransactionExecutor', () => {
[
SERVICE_UNAVAILABLE,
TRANSIENT_ERROR_2,
'Neo.ClientError.Security.AuthorizationExpired',
SESSION_EXPIRED,
SESSION_EXPIRED
],
Expand Down
3 changes: 2 additions & 1 deletion testkit-backend/src/skipped-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ const skippedTests = [
),
ifEndsWith(
'test_should_retry_write_until_success_with_leader_change_using_tx_function'
)
),
ifEndsWith('test_should_retry_on_auth_expired_on_begin_using_tx_function')
),
skip(
'Driver is creating a dedicated connection to check the MultiDBSupport',
Expand Down