IBM / nodejs-idb-connector

A JavaScript (Node.js) library for communicating with Db2 for IBM i, with support for queries, procedures, and much more. Uses traditional callback-style syntax
MIT License
37 stars 23 forks source link

Possible memory leak? #90

Closed codypace68 closed 4 years ago

codypace68 commented 5 years ago

The below code is causing some unexpected results. Note this only occurs when running a sql statement on loop. Also, i realize this isn't an eloquent way to run a statement on loop and am using this for testing only.

So the problem I'm having is that the temp storage of job running this code grows forever and ever and never decreases (along with disk usage). If this isn't a leak do you have any idea why this behavior is happening?

Additionally I'm logging memory usage every 5 iterations and it appears that the heap usage and heap total are rising and falling as one would would expect with setTimeout()/setInterval(). And the rss remains 0. this would normally mean no memory leak. Except the temp storage and disk usage continue to climb.

Also, I know every second is far too often to run this code. But in production i run the same process every 60 seconds and am seeing the same rise in tempStorage/ASP usage at a much slower rate.

Finally, over about 100 iterations the temp storage rises to about 1300MB and the disk usage increases by almost a GB.

Things I've tried so far with exactly the same results:

  1. moving connection object into the function and creating and destroying the object each iteration.
  2. Having a setInterval outside of the function to call the function instead of a setTimeout inside the function.
  3. Setting all variable to null to try to force GC to pick them up.
  4. Actually expose GC and run every 5th iteration. Shows no decrease in TempStorage or Disk Usage.
const { dbconn, dbstmt } = require('idb-connector');// require idb-connector
let connection = new dbconn();
connection.conn("*LOCAL");
let i = 0

test();
function test() {
    i += 1;
    if (i % 5 === 0) {
        console.log(process.memoryUsage());
    }
    let statement = new dbstmt(connection);
    const sSql = 'SELECT * 
        FROM table';

    var data = statement.execSync(sSql, (rows, err) => {
      if (err) {
        throw err;
      }
      runAgain();
    });
    statement.close(); // Clean up the statement object.
};

function runAgain() {
    setTimeout(test, 1000)
}

Memory usage log over 25 iterations. { rss: 0,
heapTotal: 37761024,
heapUsed: 26927184,
external: 8680 }
{ rss: 0, heapTotal: 33566720, heapUsed: 7505504, external: 8680 } { rss: 0,
heapTotal: 28848128,
heapUsed: 19293008,
external: 8680 }
{ rss: 0, heapTotal: 25702400, heapUsed: 7505520, external: 8680 } { rss: 0,
heapTotal: 35663872,
heapUsed: 14556624,
external: 8680 }

dmabupt commented 5 years ago

Hello @codypace68 , May I ask the version of your Node.js/idb-connector? On my test system with Node.js v12.8.1, the V8 GC works well when heapUsed hit 4565112 bytes -->

...
{ rss: 0, heapTotal: 7467008, heapUsed: 4556360, external: 912860 }
{ rss: 0, heapTotal: 7467008, heapUsed: 4565112, external: 912860 }
{ rss: 0, heapTotal: 7467008, heapUsed: 3601048, external: 912860 }
{ rss: 0, heapTotal: 7467008, heapUsed: 3609800, external: 912860 }
...
codypace68 commented 5 years ago

Node.js v10.15.0 idb-connector@1.2.0

And I'm not seeing heap usage growth either. I'm only seeing Temp Storage growth from the wrkactjob screen

codypace68 commented 5 years ago

Any updates on this issue?

dmabupt commented 4 years ago

@codypace68 I can recreate the issue and now investigating it.

dmabupt commented 4 years ago

Found the root cause and now evaluating the impact of the patch. I will deliver a fix for it later.

dmabupt commented 4 years ago

I just released v1.2.3 for this fix. @codypace68 you may update to this version and verify it.

The essential code change is this line -- https://github.com/IBM/nodejs-idb-connector/commit/61dd9e461d931a3824c5dfa1ad06386551b63136#diff-b307b5c576890c839b53066078db965cR2107

codypace68 commented 4 years ago

Perfect! Thank you so much for your help. I’ll update the package tomorrow and report back with the results.

Sent from my iPhone

On Oct 14, 2019, at 11:12 PM, Xu Meng notifications@github.com wrote:

 I just released v1.2.3 for this fix. @codypace68 you may update to this version and verify it.

The essential code change is this line -- 61dd9e4#diff-b307b5c576890c839b53066078db965cR2107

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

codypace68 commented 4 years ago

Initial testing seems to show positive results. Temp storage is 177 over 24 hours as opposed to over 400.

codypace68 commented 4 years ago

If you added a callback option to the close method you could call the next iteration only if the previous statement had closed. That should fix this issue shouldn’t it?

Sent from my iPhone

On Oct 17, 2019, at 3:30 AM, Xu Meng notifications@github.com wrote:

 @codypace68 The leak issue in execSync() has been resolved. But there seems to be leak in the constructor of the dbstmt object. Below code consumes more memory --

for(let i = 0; i < 10000; i++) { let statement = new dbstmt(connection); statement.execSync(sSql); statement.closeCursor(); statement.close(); }; than

let statement = new dbstmt(connection); for(let i = 0; i < 10000; i++) { statement.execSync(sSql); statement.closeCursor(); }; statement.close(); Seems the statement objects are not deallocated in time.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

dmabupt commented 4 years ago

@codypace68 Unfortunately it does not help. Actually, even I just run below "do nothing" code, the memory usage still increases --

for(let i = 0; i < 10000; i++) {
    let statement = new dbstmt(connection);
    if(i % 100 === 0) console.log(`Run ${i} times`);
    statement.close();
};
codypace68 commented 4 years ago

That is strange. The changes you made helped a lot though. I’ve seen a significant decrease in temp storage/ memory usage since the last update. So it’s better than it was for sure

Sent from my iPhone

On Oct 17, 2019, at 10:33 PM, Xu Meng notifications@github.com wrote:

 @codypace68 Unfortunatelu it does not help. Actually, even I just run below "do nothing" code, the memory usage still increases --

for(let i = 0; i < 10000; i++) { let statement = new dbstmt(connection); if(i % 100 === 0) console.log(Run ${i} times); statement.close(); }; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

github-actions[bot] commented 4 years ago

:wave: Hi! This issue has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed.