Skip to content

Commit d5bd032

Browse files
authored
Fix OOM crash, when closing a transaction while Queries are still ongoing (#1193)
When closing a transaction while queries are still running, the `_onErrorCallbacks` will cause an infinite loop which leads to an OOM crash. Because `FAILED` is only set as a state in this function, I opted to use it as a failsafe check, to cut the recursion short. The test triggers the OOM crash when the fix is not included, otherwise it passes in <1s on my machine. I hope the test is ok as it is, let me know if you need any changes for this. EDIT: Upon a bit further testing, it's not actually infinite, but just scaling exponentially. 3 simultaneous queries gets us ~3000 `_onErrorCallback` calls, 4 gets us ~32000, 5 gets us ~195000, 6 gets us ~840000, 7-12 gets us "Map maximum size exceeded" 13+ gets us the mentioned OOM
1 parent 4663360 commit d5bd032

File tree

3 files changed

+37
-0
lines changed

3 files changed

+37
-0
lines changed

packages/core/src/transaction.ts

+6
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ class Transaction {
299299
// error will be "acknowledged" by sending a RESET message
300300
// database will then forget about this transaction and cleanup all corresponding resources
301301
// it is thus safe to move this transaction to a FAILED state and disallow any further interactions with it
302+
303+
if (this._state === _states.FAILED) {
304+
// already failed, nothing to do
305+
// if we call onError for each result again, we might run into an infinite loop, that causes an OOM eventually
306+
return Promise.resolve(null)
307+
}
302308
this._state = _states.FAILED
303309
this._onClose()
304310
this._results.forEach(result => {

packages/neo4j-driver-deno/lib/core/transaction.ts

+6
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ class Transaction {
299299
// error will be "acknowledged" by sending a RESET message
300300
// database will then forget about this transaction and cleanup all corresponding resources
301301
// it is thus safe to move this transaction to a FAILED state and disallow any further interactions with it
302+
303+
if (this._state === _states.FAILED) {
304+
// already failed, nothing to do
305+
// if we call onError for each result again, we might run into an infinite loop, that causes an OOM eventually
306+
return Promise.resolve(null)
307+
}
302308
this._state = _states.FAILED
303309
this._onClose()
304310
this._results.forEach(result => {

packages/neo4j-driver/test/transaction.test.js

+25
Original file line numberDiff line numberDiff line change
@@ -499,4 +499,29 @@ describe('#integration transaction', () => {
499499
.catch(done.fail.bind(done))
500500
)
501501
}
502+
503+
/**
504+
* This test is to verify that the driver does not run out of memory when one of the queries fails,
505+
* while others are concurrently running and the transaction is being committed/closed.
506+
*
507+
* Because this causes an infinite loop in the failure case, the test has a timeout of 50 seconds, in order to actualy
508+
* trip the OOM error, and not run into a timeout error prematurely.
509+
*/
510+
it('transactions should not run OOM when one of the queries fails', async () => {
511+
const transaction = await session.beginTransaction()
512+
const queries = Array.from(
513+
{ length: 10 },
514+
(_, i) => () => transaction.run(`RETURN ${i}`)
515+
)
516+
const destroy = () => transaction.close()
517+
518+
const calls = [...queries, destroy, ...queries]
519+
const promises = calls.map(call => call())
520+
try {
521+
await Promise.all(promises)
522+
} catch (e) {
523+
// This is the success case, as we god an exception, not run into an OOM
524+
expect(e.message).toBe('Cannot run query in this transaction, because it has already been rolled back.')
525+
}
526+
}, 50000)
502527
})

0 commit comments

Comments
 (0)