Skip to content
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

Missing queued cbs when call pool.end() #797

Closed
iammapping opened this issue Apr 25, 2014 · 8 comments
Closed

Missing queued cbs when call pool.end() #797

iammapping opened this issue Apr 25, 2014 · 8 comments
Labels
Milestone

Comments

@iammapping
Copy link

The callbacks in pool._connectionQueue would never been executed, when calling pool.end(). I think it is necessary to append the under segment to function Pool.prototype.end

while (this._connectionQueue.length > 0) {
  // tell callback in queue that the pool is ended
  this._connectionQueue.shift()(new Error('Pool is ended'), null);
}
@dougwilson dougwilson added this to the 2.2 milestone Apr 25, 2014
@dougwilson dougwilson added the bug label Apr 25, 2014
@dougwilson dougwilson changed the title Perhaps memory leak when call pool.end() Missing queued cbs when call pool.end() Apr 25, 2014
@dougwilson
Copy link
Member

I altered your issue title because it's not a memory leak; not calling callbacks does not leak memory, that's not how JavaScript works. But I do see what you mean about the connection queue. I've scheduled the fix for 2.2.

@iammapping
Copy link
Author

@dougwilson Sorry. I didn't explain clearly. This may cause user's program leaking memory, such as:

var mysql = require('mysql');
var pool  = null;
var count = 1;

var createPool = function() {
  // recreate pool while it is closed
  if (!pool) {
    pool = mysql.createPool({
      host: '127.0.0.1',
      user: 'root',
      password: '',
      connectionLimit: 1
    })
  }
};


setInterval(function(req, res) {
  // allocate a large array
  var size = 10 * 1024 * 1024;
  var arr = new Array(size);
  for (var i = 0; i < size; i++) {
    arr[i] = 0;
  }

  createPool();

  // make sure the other callbacks pushed into connection queue
  pool.query('SELECT SLEEP(1000)', function(err) {
    // free the large array in callback
    arr = null;
    if (err) {
      // console.log(err)
      pool = null;
    }
  });

  // call pool.end every 5 times
  if (count % 5 === 0) {
    pool && pool.end(function(err) {});
  }

  // print memory usage
  var mem = process.memoryUsage();
  var format = function(bytes) {
    return (bytes / 1024 / 1024).toFixed(2) + 'MB';
  };
  console.log('Process ' + count + ': heapTotal ' + format(mem.heapTotal) + ' heapUsed ' + format(mem.heapUsed) + ' rss ' + format(mem.rss));
  console.log('-----------------------------------------------------');

  count++
}, 10);

@dougwilson
Copy link
Member

This is fixed now.

@iammapping
Copy link
Author

@dougwilson Hi, I'm looking at your last commit, but the bug I posted still exists. The callbacks in pool's connection queue would never been executed. I wrote a demo to reappear, you can run the under code:

var mysql = require('mysql');
var pool  = null;
var count = 1;

pool = mysql.createPool({
  host: '127.0.0.1',
  user: 'root',
  password: '',
  connectionLimit: 1
});

var interval = setInterval(function(req, res) {
  (function(cbIndex) {
    // make sure the other callbacks pushed into connection queue
    pool.query('SELECT SLEEP(10)', function(err) {
      if (err) {
        console.log(cbIndex + ' ' + err);
      }  else {
        console.log(cbIndex + ' exec');
      }
    });
  })(count);

  // call pool.end when arrived 5 
  if (count % 5 === 0) {
    pool.end(function(err) {});
  }

  // stop
  if (count === 10) {
    clearInterval(interval);
  }

  count++;
}, 10);

The result:
image
The callbacks from 3 to 5 have been pushed into connection queue, but they are not executed.

@dougwilson
Copy link
Member

Thanks :) I've identified the issue, and will have it fixed soon. I appreciate you checking that the fixes are actually working for you 👍

@dougwilson
Copy link
Member

OK, now the the latest commit 1397f67 your example script is running on the callbacks. Let me know if you find anything else :)

@iammapping
Copy link
Author

Great, it works.

@dougwilson
Copy link
Member

Awesome to hear 👏 it's even in the new 2.2.0 release, so you can officially have this fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants