Skip to content

Commit 2e078e6

Browse files
lutovichlutovich
authored
Merge pull request #223 from lutovich/1.2-no-retry-when-terminated
Do not retry transaction when it is terminated
2 parents faca37c + 28a4e95 commit 2e078e6

File tree

6 files changed

+230
-31
lines changed

6 files changed

+230
-31
lines changed

‎src/v1/internal/transaction-executor.js

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,23 @@ export default class TransactionExecutor {
122122
return error && error.code &&
123123
(error.code === SERVICE_UNAVAILABLE ||
124124
error.code === SESSION_EXPIRED ||
125-
error.code.indexOf('TransientError') >= 0);
125+
this._isTransientError(error));
126+
}
127+
128+
static _isTransientError(error) {
129+
// Retries should not happen when transaction was explicitly terminated by the user.
130+
// Termination of transaction might result in two different error codes depending on where it was
131+
// terminated. These are really client errors but classification on the server is not entirely correct and
132+
// they are classified as transient.
133+
134+
const code = error.code;
135+
if (code.indexOf('TransientError') >= 0) {
136+
if (code === 'Neo.TransientError.Transaction.Terminated' || code === 'Neo.TransientError.Transaction.LockClientStopped') {
137+
return false;
138+
}
139+
return true;
140+
}
141+
return false;
126142
}
127143

128144
_verifyAfterConstruction() {

‎test/internal/transaction-executor.test.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import {hijackNextDateNowCall, setTimeoutMock} from './timers-util';
2424
const TRANSIENT_ERROR_1 = 'Neo.TransientError.Transaction.DeadlockDetected';
2525
const TRANSIENT_ERROR_2 = 'Neo.TransientError.Network.CommunicationError';
2626
const UNKNOWN_ERROR = 'Neo.DatabaseError.General.UnknownError';
27+
const TX_TERMINATED_ERROR = 'Neo.TransientError.Transaction.Terminated';
28+
const LOCKS_TERMINATED_ERROR = 'Neo.TransientError.Transaction.LockClientStopped';
2729
const OOM_ERROR = 'Neo.DatabaseError.General.OutOfMemoryError';
2830

2931
describe('TransactionExecutor', () => {
@@ -62,6 +64,14 @@ describe('TransactionExecutor', () => {
6264
testNoRetryOnUnknownError([UNKNOWN_ERROR], 1, done);
6365
});
6466

67+
it('should not retry when transaction work returns promise rejected with transaction termination error', done => {
68+
testNoRetryOnUnknownError([TX_TERMINATED_ERROR], 1, done);
69+
});
70+
71+
it('should not retry when transaction work returns promise rejected with locks termination error', done => {
72+
testNoRetryOnUnknownError([LOCKS_TERMINATED_ERROR], 1, done);
73+
});
74+
6575
it('should stop retrying when time expires', done => {
6676
const executor = new TransactionExecutor();
6777
let workInvocationCounter = 0;

‎test/resources/boltkit/acquire_endpoints_and_exit.script

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
!: AUTO INIT
2+
!: AUTO RESET
3+
!: AUTO PULL_ALL
4+
5+
C: RUN "CALL dbms.cluster.routing.getServers" {}
6+
PULL_ALL
7+
S: SUCCESS {"fields": ["ttl", "servers"]}
8+
RECORD [9223372036854775807, [{"addresses": ["127.0.0.1:9007","127.0.0.1:9008"],"role": "WRITE"}, {"addresses": ["127.0.0.1:9005","127.0.0.1:9006"], "role": "READ"},{"addresses": ["127.0.0.1:9001","127.0.0.1:9002","127.0.0.1:9003"], "role": "ROUTE"}]]
9+
SUCCESS {}
10+
S: <EXIT>

‎test/resources/boltkit/read_server_and_exit.script

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
!: AUTO INIT
2+
!: AUTO RESET
3+
!: AUTO PULL_ALL
4+
!: AUTO RUN "COMMIT" {}
5+
!: AUTO RUN "ROLLBACK" {}
6+
!: AUTO RUN "BEGIN" {}
7+
8+
C: RUN "MATCH (n) RETURN n.name" {}
9+
PULL_ALL
10+
S: SUCCESS {"fields": ["n.name"]}
11+
RECORD ["Bob"]
12+
RECORD ["Alice"]
13+
RECORD ["Tina"]
14+
SUCCESS {}
15+
S: <EXIT>

‎test/v1/routing.driver.boltkit.it.js

Lines changed: 47 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1466,27 +1466,44 @@ describe('routing driver', () => {
14661466
}
14671467

14681468
const kit = new boltkit.BoltKit();
1469-
const router1 = kit.start('./test/resources/boltkit/acquire_endpoints.script', 9010);
1469+
// use scripts that exit eagerly when they are executed to simulate failed servers
1470+
const router1 = kit.start('./test/resources/boltkit/acquire_endpoints_and_exit.script', 9010);
1471+
const tmpReader = kit.start('./test/resources/boltkit/read_server_and_exit.script', 9005);
14701472

14711473
kit.run(() => {
14721474
const driver = newDriver('bolt+routing://127.0.0.1:9010');
1473-
const session = driver.session();
14741475

1475-
// restart router on the same port with different script that contains itself as reader
1476-
router1.exit(() => {
1477-
const router2 = kit.start('./test/resources/boltkit/rediscover_using_initial_router.script', 9010);
1478-
1479-
session.readTransaction(tx => tx.run('MATCH (n) RETURN n.name AS name')).then(result => {
1480-
const records = result.records;
1481-
expect(records.length).toEqual(2);
1482-
expect(records[0].get('name')).toEqual('Bob');
1483-
expect(records[1].get('name')).toEqual('Alice');
1476+
// run a dummy query to force routing table initialization
1477+
const session = driver.session(READ);
1478+
session.run('MATCH (n) RETURN n.name').then(result => {
1479+
expect(result.records.length).toEqual(3);
1480+
session.close(() => {
1481+
// stop existing router and reader
1482+
router1.exit(code1 => {
1483+
tmpReader.exit(code2 => {
1484+
// at this point previously used router and reader should be dead
1485+
expect(code1).toEqual(0);
1486+
expect(code2).toEqual(0);
14841487

1485-
session.close(() => {
1486-
driver.close();
1487-
router2.exit(code => {
1488-
expect(code).toEqual(0);
1489-
done();
1488+
// start new router on the same port with different script that contains itself as reader
1489+
const router2 = kit.start('./test/resources/boltkit/rediscover_using_initial_router.script', 9010);
1490+
1491+
kit.run(() => {
1492+
session.readTransaction(tx => tx.run('MATCH (n) RETURN n.name AS name')).then(result => {
1493+
const records = result.records;
1494+
expect(records.length).toEqual(2);
1495+
expect(records[0].get('name')).toEqual('Bob');
1496+
expect(records[1].get('name')).toEqual('Alice');
1497+
1498+
session.close(() => {
1499+
driver.close();
1500+
router2.exit(code => {
1501+
expect(code).toEqual(0);
1502+
done();
1503+
});
1504+
});
1505+
});
1506+
});
14901507
});
14911508
});
14921509
});
@@ -1502,28 +1519,28 @@ describe('routing driver', () => {
15021519

15031520
const kit = new boltkit.BoltKit();
15041521
const router1 = kit.start('./test/resources/boltkit/acquire_endpoints.script', 9010);
1522+
// start new router on a different port to emulate host name resolution
1523+
// this router uses different script that contains itself as reader
1524+
const router2 = kit.start('./test/resources/boltkit/rediscover_using_initial_router.script', 9009);
15051525

15061526
kit.run(() => {
15071527
const driver = newDriver('bolt+routing://127.0.0.1:9010');
15081528
// make seed address resolve to 3 different addresses (only last one has backing stub server):
15091529
setupFakeHostNameResolution(driver, '127.0.0.1:9010', ['127.0.0.1:9011', '127.0.0.1:9012', '127.0.0.1:9009']);
15101530
const session = driver.session();
15111531

1512-
// start new router on a different port to emulate host name resolution
1513-
// this router uses different script that contains itself as reader
1514-
router1.exit(() => {
1515-
const router2 = kit.start('./test/resources/boltkit/rediscover_using_initial_router.script', 9009);
1516-
1517-
session.readTransaction(tx => tx.run('MATCH (n) RETURN n.name AS name')).then(result => {
1518-
const records = result.records;
1519-
expect(records.length).toEqual(2);
1520-
expect(records[0].get('name')).toEqual('Bob');
1521-
expect(records[1].get('name')).toEqual('Alice');
1532+
session.readTransaction(tx => tx.run('MATCH (n) RETURN n.name AS name')).then(result => {
1533+
const records = result.records;
1534+
expect(records.length).toEqual(2);
1535+
expect(records[0].get('name')).toEqual('Bob');
1536+
expect(records[1].get('name')).toEqual('Alice');
15221537

1523-
session.close(() => {
1524-
driver.close();
1525-
router2.exit(code => {
1526-
expect(code).toEqual(0);
1538+
session.close(() => {
1539+
driver.close();
1540+
router1.exit(code1 => {
1541+
router2.exit(code2 => {
1542+
expect(code1).toEqual(0);
1543+
expect(code2).toEqual(0);
15271544
done();
15281545
});
15291546
});

‎test/v1/session.test.js

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,120 @@ describe('session', () => {
796796
});
797797
});
798798

799+
it('should interrupt query waiting on a lock when closed', done => {
800+
if (!serverIs31OrLater(done)) {
801+
// locks are transaction termination aware by default only in 3.1+
802+
return;
803+
}
804+
805+
session.run('CREATE ()').then(() => {
806+
session.close(() => {
807+
const session1 = driver.session();
808+
const session2 = driver.session();
809+
const tx1 = session1.beginTransaction();
810+
811+
tx1.run('MATCH (n) SET n.id = 1 RETURN 42 AS answer').then(result => {
812+
// node is now locked by tx1
813+
expect(result.records.length).toEqual(1);
814+
expect(result.records[0].get(0).toNumber()).toEqual(42);
815+
816+
// this query should get stuck waiting for the lock
817+
session2.run('MATCH (n) SET n.id = 2 RETURN 42 AS answer').catch(error => {
818+
expectTransactionTerminatedError(error);
819+
tx1.commit().then(() => {
820+
readAllNodeIds().then(ids => {
821+
expect(ids).toEqual([1]);
822+
done();
823+
});
824+
});
825+
});
826+
827+
setTimeout(() => {
828+
// close session after a while
829+
session2.close();
830+
}, 1000);
831+
});
832+
});
833+
});
834+
});
835+
836+
it('should interrupt transaction waiting on a lock when closed', done => {
837+
if (!serverIs31OrLater(done)) {
838+
// locks are transaction termination aware by default only in 3.1+
839+
return;
840+
}
841+
842+
session.run('CREATE ()').then(() => {
843+
session.close(() => {
844+
const session1 = driver.session();
845+
const session2 = driver.session();
846+
const tx1 = session1.beginTransaction();
847+
const tx2 = session2.beginTransaction();
848+
849+
tx1.run('MATCH (n) SET n.id = 1 RETURN 42 AS answer').then(result => {
850+
// node is now locked by tx1
851+
expect(result.records.length).toEqual(1);
852+
expect(result.records[0].get(0).toNumber()).toEqual(42);
853+
854+
// this query should get stuck waiting for the lock
855+
tx2.run('MATCH (n) SET n.id = 2 RETURN 42 AS answer').catch(error => {
856+
expectTransactionTerminatedError(error);
857+
tx1.commit().then(() => {
858+
readAllNodeIds().then(ids => {
859+
expect(ids).toEqual([1]);
860+
done();
861+
});
862+
});
863+
});
864+
865+
setTimeout(() => {
866+
// close session after a while
867+
session2.close();
868+
}, 1000);
869+
});
870+
});
871+
});
872+
});
873+
874+
it('should interrupt transaction function waiting on a lock when closed', done => {
875+
if (!serverIs31OrLater(done)) {
876+
// locks are transaction termination aware by default only in 3.1+
877+
return;
878+
}
879+
880+
session.run('CREATE ()').then(() => {
881+
session.close(() => {
882+
const session1 = driver.session();
883+
const session2 = driver.session();
884+
const tx1 = session1.beginTransaction();
885+
886+
tx1.run('MATCH (n) SET n.id = 1 RETURN 42 AS answer').then(result => {
887+
// node is now locked by tx1
888+
expect(result.records.length).toEqual(1);
889+
expect(result.records[0].get(0).toNumber()).toEqual(42);
890+
891+
session2.writeTransaction(tx2 => {
892+
// this query should get stuck waiting for the lock
893+
return tx2.run('MATCH (n) SET n.id = 2 RETURN 42 AS answer').catch(error => {
894+
expectTransactionTerminatedError(error);
895+
tx1.commit().then(() => {
896+
readAllNodeIds().then(ids => {
897+
expect(ids).toEqual([1]);
898+
done();
899+
});
900+
});
901+
});
902+
});
903+
904+
setTimeout(() => {
905+
// close session after a while
906+
session2.close();
907+
}, 1000);
908+
});
909+
});
910+
});
911+
});
912+
799913
function serverIs31OrLater(done) {
800914
// lazy way of checking the version number
801915
// if server has been set we know it is at least 3.1
@@ -840,4 +954,21 @@ describe('session', () => {
840954
expect(bookmark).toBeDefined();
841955
expect(bookmark).not.toBeNull();
842956
}
957+
958+
function expectTransactionTerminatedError(error) {
959+
expect(error.message.toLowerCase().indexOf('transaction terminated') >= 0).toBeTruthy();
960+
}
961+
962+
function readAllNodeIds() {
963+
return new Promise((resolve, reject) => {
964+
const session = driver.session();
965+
session.run('MATCH (n) RETURN n.id').then(result => {
966+
const ids = result.records.map(record => record.get(0).toNumber());
967+
session.close();
968+
resolve(ids);
969+
}).catch(error => {
970+
reject(error);
971+
});
972+
});
973+
}
843974
});

0 commit comments

Comments
 (0)