Fix OOM crash, when closing a transaction while Queries are still ongoing (#1193)

reckter · web-flow · commit d5bd0323e4a2 · 2024-05-29T13:31:31.000+02:00
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 &lt;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
diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts
@@ -299,6 +299,12 @@ class Transaction {
     // error will be "acknowledged" by sending a RESET message
     // database will then forget about this transaction and cleanup all corresponding resources
     // it is thus safe to move this transaction to a FAILED state and disallow any further interactions with it
+
+    if (this._state === _states.FAILED) {
+      // already failed, nothing to do
+      // if we call onError for each result again, we might run into an infinite loop, that causes an OOM eventually
+      return Promise.resolve(null)
+    }
     this._state = _states.FAILED
     this._onClose()
     this._results.forEach(result => {
diff --git a/packages/neo4j-driver-deno/lib/core/transaction.ts b/packages/neo4j-driver-deno/lib/core/transaction.ts
@@ -299,6 +299,12 @@ class Transaction {
     // error will be "acknowledged" by sending a RESET message
     // database will then forget about this transaction and cleanup all corresponding resources
     // it is thus safe to move this transaction to a FAILED state and disallow any further interactions with it
+
+    if (this._state === _states.FAILED) {
+      // already failed, nothing to do
+      // if we call onError for each result again, we might run into an infinite loop, that causes an OOM eventually
+      return Promise.resolve(null)
+    }
     this._state = _states.FAILED
     this._onClose()
     this._results.forEach(result => {
diff --git a/packages/neo4j-driver/test/transaction.test.js b/packages/neo4j-driver/test/transaction.test.js
@@ -499,4 +499,29 @@ describe('#integration transaction', () => {
           .catch(done.fail.bind(done))
       )
   }
+
+  /**
+     * This test is to verify that the driver does not run out of memory when one of the queries fails,
+     * while others are concurrently running and the transaction is being committed/closed.
+     *
+     * Because this causes an infinite loop in the failure case, the test has a timeout of 50 seconds, in order to actualy
+     * trip the OOM error, and not run into a timeout error prematurely.
+     */
+  it('transactions should not run OOM when one of the queries fails', async () => {
+    const transaction = await session.beginTransaction()
+    const queries = Array.from(
+      { length: 10 },
+      (_, i) => () => transaction.run(`RETURN ${i}`)
+    )
+    const destroy = () => transaction.close()
+
+    const calls = [...queries, destroy, ...queries]
+    const promises = calls.map(call => call())
+    try {
+      await Promise.all(promises)
+    } catch (e) {
+      // This is the success case, as we god an exception, not run into an OOM
+      expect(e.message).toBe('Cannot run query in this transaction, because it has already been rolled back.')
+    }
+  }, 50000)
 })
