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

Connections are not automatically closed #8468

Closed
Vadskye opened this issue Oct 11, 2017 · 44 comments
Closed

Connections are not automatically closed #8468

Vadskye opened this issue Oct 11, 2017 · 44 comments

Comments

@Vadskye
Copy link

@Vadskye Vadskye commented Oct 11, 2017

What are you doing?

Assume $DATABASE_URL points to a real Postgres database.

const Sequelize = require('sequelize');

function main() {
  const sequelize = new Sequelize(
    process.env.DATABASE_URL,
    {dialect: 'postgres', operatorsAliases: false}
  );

  return sequelize.query(`select 1`);
}

main();

Full environment details and demo available at https://github.com/Vadskye/demo-sequelize.

What do you expect to happen?

The script should finish its execution 10 seconds after the query finishes, when the connection times out and all active connections have been closed. This was the behavior with Sequelize v3.

What is actually happening?

The script hangs indefinitely after the query finishes.

Script output with DEBUG=* sequelize:pool pool created with max/min: 5/0, no replication +0ms

sequelize:connection:pg connection acquired +0ms
sequelize:sql:pg executing(default) : SHOW SERVER_VERSION +0ms
sequelize:sql:pg executed(default) : SHOW SERVER_VERSION +1ms
sequelize:connection:pg connection timeout +20ms
sequelize:connection:pg connection acquired +4ms
sequelize:pool connection acquired +62ms
Executing (default): select 1
sequelize:sql:pg executing(default) : select 1 +27ms
sequelize:sql:pg executed(default) : select 1 +1ms
sequelize:pool connection released +6ms
sequelize:connection:pg connection timeout +10s
sequelize:pool connection destroy +10s

Dialect: postgres
Dialect version: 6.4.2
Database version: 9.4
Sequelize version: 4.13.8
Tested with master branch: No; there is one commit to master since the last release, and it does not affect this issue.

@hussainb
Copy link

@hussainb hussainb commented Oct 20, 2017

Shouldn't this be of major concern.. Any timeline on the fix?

@everthis
Copy link

@everthis everthis commented Nov 15, 2017

Is there a way to exit the process automatically without sequelize.close() ?

@jjm340
Copy link

@jjm340 jjm340 commented Nov 24, 2017

+1

@rsshilli
Copy link

@rsshilli rsshilli commented Dec 15, 2017

We're on MySQL and just upgraded from Sequelize 3 to Sequelize 4. We're using Node 6.10.3 running inside of AWS Lambdas, and this ate up 2 days of our time to track down. Our Lambdas always timeout now, but used to respond immediately using Sequelize 3.

At the very least, the upgrade docs should mention that you need to call sequelize.close() at the end of every block of promises. Also, if sequelize.close() means I can't use the sequelize instance any more, you should also mention that Sequelize 4 can't really be used effectively in AWS Lambda.

@igorescobar
Copy link

@igorescobar igorescobar commented Jan 31, 2018

Also running sequelize 4 inside of a lambda and having major issues related the problem mentioned by @rsshilli

@liviuignat
Copy link

@liviuignat liviuignat commented Mar 20, 2018

How is this possible this is still open? This looks like a REALLY major issue in Sequelize.

@igorescobar
Copy link

@igorescobar igorescobar commented Mar 20, 2018

I had to migrate to MongoDB to be able to use any kind of persistence layer on AWS Lambda structure.

@stevenking86
Copy link

@stevenking86 stevenking86 commented Jun 11, 2018

Do older versions of sequelize also have this issue?

@rsshilli
Copy link

@rsshilli rsshilli commented Jun 11, 2018

@stevenking86 No, they don't. Sequelize 3 works great on Lambda.

@stevenking86
Copy link

@stevenking86 stevenking86 commented Jun 11, 2018

@rsshilli thanks, going to use Sequelize 3 then!

@catamphetamine
Copy link

@catamphetamine catamphetamine commented Aug 4, 2018

We have been having this issue too with Amazon Lambda.
We tried two workarounds for this.

The first one:

// Suspend the Node.js process immediately after response is sent.
// Fixes Sequelize connection pool preventing Node.js process from terminating.
context.callbackWaitsForEmptyEventLoop = false;

It does work.
But it's a very hacky approach and feels a bit like a ticking bomb.

The second approach we tried (which didn't work) is based on #7144 (comment)

try {
  ... some DB-related stuff ...
} catch (error) {
  callback(JSON.stringify({
    code: 500,
    message: 'Something went wrong...'
  }));
} finally {
  await sequelize.connectionManager._onProcessExit();
}

_onProcessExit is a non-documented hidden function.
In fact, originally is was called onProcessExit but then was renamed to _onProcessExit in 5.0.0 beta, perhaps to emphasize its undocumentedness.

What _onProcessExit does is:

  /**
   * Handler which executes on process exit or connection manager shutdown
   *
   * @private
   * @returns {Promise}
   */
  _onProcessExit() {
    if (!this.pool) {
      return Promise.resolve();
    }

    return this.pool.drain().then(() => {
      debug('connection drain due to process exit');
      return this.pool.clear();
    });
  }

So one may as well replace its call with:

finally {
  // Fixes Sequelize connection pool preventing Node.js process from terminating.
  // https://github.com/sequelize/sequelize/issues/8468#issuecomment-410451242
  const pool = sequelize.connectionManager.pool;
  if (pool) {
    await pool.drain();
    await pool.clear();
  }
}

To protect the code from another _onProcessExit method rename.

This looked like it could work, and it did work for a single Lambda invocation: the lambda executed and returned the result, but if the user hit "Refresh" button in the web browser the lambda would throw an error: pool is draining and cannot accept work.

So, in the end, the "dran() and clear()" workaround didn't work: after calling pool.drain() the pool seems to be unusable anymore so next time when the Lambda gets reused (next request comes shortly after the previous one) it throws pool is draining and cannot accept work.
https://github.com/coopernurse/node-pool#draining

Commenting out pool.drain() and leaving just pool.clear() didn't work — the Lamba just hangs in this case and timeouts as a result.

I guess there's no way in Sequelize to make it work with Lambda properly then.
The only workaround is the unsafe context.callbackWaitsForEmptyEventLoop = false hack.

@AlastairTaft
Copy link
Contributor

@AlastairTaft AlastairTaft commented Sep 12, 2018

Also using this on aws lambda with postgres and found an issue in the code where the execution would end before the connection is closed. I've fixed it in this pull request.

Other things I did which may or may not help your use case:

  • Added the database pool options evict: 0, handleDisconnects: false.
  • Like one of the above comments, instead of running await db.sequelize.close(), instead ran const pool = db.sequelize.connectionManager.pool; await pool.drain(); await pool.clear();.
  • Also set the option databaseVersion: '9.6.6', which meant that it avoids an extra query to the database on every request to figure out what version is in use.

And here's my monkey patch without having to run a modified version of sequelize

sequelize.connectionManager.disconnect = connection => ({
  tap: (method) => connection.end().then(method),
})
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 13, 2018

Sorry for ignoring this issue for long time.

I know this is an important breaking change but Sequelize can't do anything here. node-pool implementation keeps the pool alive thus preventing clean shutdown. For example

const Pool = require('generic-pool');

const _connect = () => {
  return {
    name: Math.random().toString()
  };
}

const _validate = connection => {
  return !!connection.name;
}

// Using most of Sequelize config
const pool = Pool.createPool({
  create: _connect,
  destroy: () => {},
  validate: _validate
}, {
  testOnBorrow: true,
  returnToHead: true,
  autostart: false,
  max: 2,
  min: 0,
  acquireTimeoutMillis: 10000,
  idleTimeoutMillis: 10000,
  evictionRunIntervalMillis: 10000
});

async function main () {
  const connection = await pool.acquire();

  console.log('acquire');
  
  // release connection at 5th sec
  setTimeout(async () => {
    await pool.release(connection);
    console.log('released');
  }, 5000);
 
  // pool should be empty at 15th second, this is last event loop work from user-land side
  // after finishing this pool should cleanly exit
  setTimeout(() => {
    console.log(pool.available, pool.size);
  }, 15000);
}

main();
acquire
released
0 0

In v2 version of generic-pool process would have exited at 15th second, but in v3 it will keep pool alive. Apart from that I suspect pool has memory leak.

Do you guys have any suggestion for alternative pooling library OR we can create a pooling system for Sequelize itself (may be based on v2 version of generic-pool)

@Vadskye
Copy link
Author

@Vadskye Vadskye commented Sep 13, 2018

Thanks for the response! That difficulty makes sense - it's not immediately obvious to me how node-pool would differentiate "this application has ceased all work" from "no work happens to have been done within the past N seconds". Can Sequelize just downgrade to node-pool v2?

@sushantdhiman sushantdhiman added this to the v5 milestone Sep 15, 2018
@sethetter
Copy link

@sethetter sethetter commented Oct 9, 2018

@sushantdhiman I noticed that knex switched to tarn for their pooling, due to the same issues it seems -> knex/knex#2450

@WooDzu
Copy link

@WooDzu WooDzu commented Oct 10, 2018

Not sure if related but I'm also experiencing occasional issues on Lambda with acquiring connections from a pool:

retry-as-promised:error TimeoutError: ResourceRequest timed out +4m
middleware:connection TimeoutError: ResourceRequest timed out
middleware:connection     at ResourceRequest._fireTimeout (/api/node_modules/generic-pool/lib/ResourceRequest.js:62:17)
middleware:connection     at Timeout.bound (/api/node_modules/generic-pool/lib/ResourceRequest.js:8:15)
middleware:connection     at ontimeout (timers.js:482:11)
middleware:connection     at tryOnTimeout (timers.js:317:5)
middleware:connection     at Timer.listOnTimeout (timers.js:277:5) +1ms

This would come up randomly and mass up all subsequent requests until I deploy a new lambda.

@WooDzu
Copy link

@WooDzu WooDzu commented Oct 10, 2018

I understand that pool is valuable, bit is there any way to actually disable it completely in sequelize?
I'm guessing this would fix this problem as well as reduce idle connections on mysql server.

In other words I'm considering initializing sequelize inside the lambda context rather than early and destroying it when just before sending back response. Pseudo code

module.exports = async (event, context) => {
  const db = new Sequelize({ ... });

  try {
    await db.Model.doTheJob();
    // reply 200
  } catch (e) {
    // reply 500
  } finally {
    db.destroy();
  }
};

Is this a good idea? Has anyone tried it before?

@WooDzu
Copy link

@WooDzu WooDzu commented Oct 12, 2018

I ended up with another approach to disable the pool which seems to be working so far for mysql.
I realize it is just a monkey patch but found it rather surprising a pool can't be disabled in config.

class PoolLessSequelize extends Sequelize {
  constructor(dsn, options) {
    options.handleDisconnects = false;
    super(dsn, options);

    // Disable pool completely
    this.connectionManager.pool.clear();
    this.connectionManager.pool = null;
    this.connectionManager.getConnection = function getConnection() {
      // debug sth
      return this._connect(this.config);
    };
    this.connectionManager.releaseConnection = function releaseConnection(connection) {
      // debug sth
      return this._disconnect(connection);
    };
  }
}

EDIT: Having spent a few hours testing this I did not notice any side effects (yet) and the Lambda issues are gone. The cost of re-establishing connections for each query is relatively low but considering it is a fix for reliability I won't grumble.

Would you actually consider adding a proper option for disabling the pool properly?

@hagen
Copy link

@hagen hagen commented Oct 17, 2018

Thanks @WooDzu - I'll be using this until Sequelize 4 is Lambda-compat.

My particular use case is as follows:
Am interfacing with an external system during the request, the response of which the user has to wait for; however I also want to cache the response from that system; this takes more time, which the user doesn't have to wait for. Previously, I'd use context.callbackWaitsForEmptyEventLoop = false;, which is fine for scenarios where the db work is tied directly to the request's response; however, I need to return a response in Lambda using the callback but continue to do work - in this case, caching data from another system; using context.callbackWaitsForEmptyEventLoop = false; will terminate the process once the callback is invoked, which I don't want. And using sequelize.close() doesn't work either, as it's not Lambda-friendly (or rather, stateless environment friendly) - the container is kept alive but sequelize at this point will not create any more connections.

@hagen
Copy link

@hagen hagen commented Oct 17, 2018

As an addendum, this seems to be a neater way to go about the above:

Sequelize.addHook('afterInit', function(sequelize) {
  sequelize.options.handleDisconnects = false;

  // Disable pool completely
  sequelize.connectionManager.pool.clear();
  sequelize.connectionManager.pool = null;
  sequelize.connectionManager.getConnection = function getConnection() {
    return this._connect(sequelize.config);
  };
  sequelize.connectionManager.releaseConnection = function releaseConnection(connection) {
    return this._disconnect(connection);
  };
})

... then go on to initialise sequelize instance

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 20, 2018

^ @hagen Although this skips pool (and solves lambda's issue) it is going to be costly. I only recommend this for lambda with small number of queries and definitely not for api / graphql server

Creating new connection is costly, easily 500ms+ overhead per query (AWS RDS), a bit less in same VPC but definitely matters otherwise we wouldn't have any pool to begin with :)

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 28, 2018

Fixed by #10051

@mikeringrose
Copy link

@mikeringrose mikeringrose commented Oct 30, 2018

@sushantdhiman any chance this will be cherry picked over to v4?

@mikeringrose mikeringrose mentioned this issue Oct 31, 2018
2 of 5 tasks complete
@mikeringrose
Copy link

@mikeringrose mikeringrose commented Oct 31, 2018

@sushantdhiman I pulled your changes over to the v4 branch, created a PR, and all of the tests are passing.

@cmosboss
Copy link

@cmosboss cmosboss commented Nov 14, 2018

@sushantdhiman any plan on putting this on v4?

@rsshilli
Copy link

@rsshilli rsshilli commented Nov 27, 2018

@sushantdhiman Please?

@Vadskye
Copy link
Author

@Vadskye Vadskye commented Nov 27, 2018

Making this change in v4 was discussed in #10103, and it looks like it's not going to happen because that would be a breaking change in a minor release. Makes sense to me.

@mikeringrose
Copy link

@mikeringrose mikeringrose commented Nov 27, 2018

@Vadskye i'm not sure how it would be a breaking change. according to semantic versioning a minor change is "when you add functionality in a backwards-compatible manner." it's been a while since i looked at my PR, and I'm fine with using my fork until I can migrate to v5.0, but this is the type of change, imo, that semantic versioning is here for.

@hagen
Copy link

@hagen hagen commented Nov 27, 2018

v5 looks great - I love watching my tests end... on their own! (remember, pool.min = 0)

chasenlehara pushed a commit to LBH3/lbh3.org that referenced this issue Dec 29, 2018
Chasen Le Hara
The root cause of this might be in sequelize and fixed in the next major release: sequelize/sequelize#8468
@gabegorelick
Copy link
Contributor

@gabegorelick gabegorelick commented Feb 6, 2019

I've opened #10424 to track disabling pooling in a more official way.

@robophil
Copy link

@robophil robophil commented Mar 13, 2019

As an addendum, this seems to be a neater way to go about the above:

Sequelize.addHook('afterInit', function(sequelize) {
  sequelize.options.handleDisconnects = false;

  // Disable pool completely
  sequelize.connectionManager.pool.clear();
  sequelize.connectionManager.pool = null;
  sequelize.connectionManager.getConnection = function getConnection() {
    return this._connect(sequelize.config);
  };
  sequelize.connectionManager.releaseConnection = function releaseConnection(connection) {
    return this._disconnect(connection);
  };
})

... then go on to initialise sequelize instance

@hagen After applying this, do you still go about closing the connection with sequelize.close()?

@hagen
Copy link

@hagen hagen commented Mar 13, 2019

@robophil under this method, my Lambda handler would have the following set, just after entering the handler:

context.callbackWaitsForEmptyEventLoop = false;

In this way, the Lambda doesn't bother waiting for the pool (of 1) to empty... as it never would empty.

I'm now using sequelize@next which includes a rewritten pool sequelize-pool that resolves that issue:, so have removed both the code you've quoted, and the callbackWaitsForEmptyEventLoop flag. Much neater, and so far sequelize@next (v5) has been working very well in prod.

If it wasn't clear, I'm only using Sequelize in AWS Lambda. This workaround won't/may not apply in traditional server-based applications, where you most likely do want pooling.

@AlastairTaft
Copy link
Contributor

@AlastairTaft AlastairTaft commented Mar 13, 2019

@hagen Careful removing that callbackWaitsForEmptyEventLoop flag as that means you won't be taking advantage of the pool when a container is reused. i.e. without it you will be waiting for all connections to close before the container is reused, effectively creating a new pool on every request.

@robophil
Copy link

@robophil robophil commented Mar 13, 2019

@robophil under this method, my Lambda handler would have the following set, just after entering the handler:

context.callbackWaitsForEmptyEventLoop = false;

In this way, the Lambda doesn't bother waiting for the pool (of 1) to empty... as it never would empty.

I'm now using sequelize@next which includes a rewritten pool sequelize-pool that resolves that issue:, so have removed both the code you've quoted, and the callbackWaitsForEmptyEventLoop flag. Much neater, and so far sequelize@next (v5) has been working very well in prod.

If it wasn't clear, I'm only using Sequelize in AWS Lambda. This workaround won't/may not apply in traditional server-based applications, where you most likely do want pooling.

Thanks for the response @hagen. I'm running this in a lambda also.
I'll give sequelize@next a try and see if closes properly with sequelize.close()

@hagen
Copy link

@hagen hagen commented Mar 13, 2019

Thanks @AlastairTaft. I never went to prod with v4. Although I understood what you're noting isn't actually possible under Lambda, or at least can't be obtained unless you have fairly regular invocations?

My understanding runs as follows:
The minimum pool size is 1, and v4 will keep 1 connection alive always. That also means the event loop doesn't empty. Without explicitly ending or removing the pool Sequelize creates, your Lambda will run to timeout, even after you've called back - and you will be billed until that timeout. There's no way to end your billed Lambda execution time and keep your pool alive.

If your container is reused fairly shortly after one invocation, then you'd gain the advantages of an existing pool, but both of those invocations will run to timeout. So I believe there's perf gain, but it may end up costing you dollar bucks. That and your CloudWatch logs will be full of timeout errors.

There's also a method of using Dynamo to monitor open connections, but boy, that's really taking it next level.

That is my take away at least. I'd love to be corrected!

@robophil no need for close() in sequelize@next - it'll close itself once your queries have completed.

@AlastairTaft
Copy link
Contributor

@AlastairTaft AlastairTaft commented Mar 13, 2019

@hagen That's an interesting point about cost. I overlooked that, and the bug in v4 must be costing people a fair amount of $. We patched to version 5.

It is possible, I think containers are kept around for at least 5 minutes, so if you get some frequent traffic you can be making use of the same pool (so long as you don't wait for idle connections to close).

@robophil
Copy link

@robophil robophil commented Apr 11, 2019

Just spotted this http://docs.sequelizejs.com/manual/upgrade-to-v5.html#pooling. Thanks for the contribution everyone.

@VictorBaudot
Copy link

@VictorBaudot VictorBaudot commented Apr 21, 2019

@robophil I feel like it's not really working because I still get memory leaks coming from sequelize after upgrading to v5

@selected-pixel-jameson
Copy link

@selected-pixel-jameson selected-pixel-jameson commented May 24, 2019

I'm not sure if I'm doing something incorrectly, but it seems as if my connections are not being closed based on the pool settings that I have.

I'm using the latest version of Sequelize 5 and my AWS Lambda function is using the async handler so I shouldn't need to use context.callbackWaitsForEmptyEventLoop = false.

const handler = serverless(app)
module.exports.handler = async (event, context) => {
  // you can do other things here
  const result = await handler(event, context)
  // and here
  return result
}

These are my pool settings.

"pool": {
        "max": 1,
        "min": 0,
        "idle": 1000,
        "evict": 1000
}

Yet for some reason the database connections in the RDS monitoring don't go down in accordance with the pool settings. They seem to just timeout when the Lambda is terminated after 5 minutes. Note the timeout on my lambda is 30 seconds. I believe the 5 minutes is the actual termination of how ever these are virtually spun up.

Here's graph depicting my connection counts. [https://cloudup.com/cg209VEwWCy] Note that the places where the connections are maintained is when NO requests are being made at all. It's hard to tell, but the steps going down happen every 5 minutes only. They never decrease any sooner. This causes significant issues if the maximum connections of the database is ever reached because all requests going forward for the next 5 minutes will just fail outright because the database has reached its maximum connections.

Any help or guidance with this would be greatly appreciated. Thank you!!

@Vadskye
Copy link
Author

@Vadskye Vadskye commented May 24, 2019

Since this is closed ticket that was originally about the Sequelize v4 pooling issue, it might be better to create a new issue for specifically Sequelize v5 pool issues. Since v5 has a different pooling library, I would expect that any lingering issues would probably come from a different problem and need to be resolved differently.

tiblu added a commit to citizenos/citizenos-api that referenced this issue May 29, 2019
@marcogrcr
Copy link

@marcogrcr marcogrcr commented Aug 26, 2020

For people experiencing issues in AWS Lambda, I have created a pull request that documents how to properly configure sequelize for Lambda. Hopefully you'll be able to see it in https://sequelize.org/master/manual/aws-lambda.html once it gets merged. In the meantime, please check #12642 out and provide feedback if you find any inaccuracies or space for improvement.

@selected-pixel-jameson
Copy link

@selected-pixel-jameson selected-pixel-jameson commented Dec 2, 2020

@marcogrcr First of all, thank you for posting your findings surrounding. I have implemented your suggestions from what I can tell. That being said, I run into an issue where once my database reaches the max connections and I start getting connection errors it appears as if the connections don't get closed within my lambda functions and will just persist open until the RDS instance, from what I can tell, decides to start shutting them down. Any suggestions??

I'm not sure if I'm doing something incorrectly, but it seems as if my connections are not being closed based on the pool settings that I have.

I'm using the latest version of Sequelize 5 and my AWS Lambda function is using the async handler so I shouldn't need to use context.callbackWaitsForEmptyEventLoop = false.

const handler = serverless(app)
module.exports.handler = async (event, context) => {
  // you can do other things here
  const result = await handler(event, context)
  // and here
  return result
}

These are my pool settings.

"pool": {
        "max": 1,
        "min": 0,
        "idle": 1000,
        "evict": 1000
}

Yet for some reason the database connections in the RDS monitoring don't go down in accordance with the pool settings. They seem to just timeout when the Lambda is terminated after 5 minutes. Note the timeout on my lambda is 30 seconds. I believe the 5 minutes is the actual termination of how ever these are virtually spun up.

Here's graph depicting my connection counts. [https://cloudup.com/cg209VEwWCy] Note that the places where the connections are maintained is when NO requests are being made at all. It's hard to tell, but the steps going down happen every 5 minutes only. They never decrease any sooner. This causes significant issues if the maximum connections of the database is ever reached because all requests going forward for the next 5 minutes will just fail outright because the database has reached its maximum connections.

Any help or guidance with this would be greatly appreciated. Thank you!!

@marcogrcr
Copy link

@marcogrcr marcogrcr commented Dec 3, 2020

@selected-pixel-jameson As per #12642, in theory, calling await sequelize.connectionManager.close(); should close all the underlying connections. If the Lambda function times out, returns before the connections are actually closed, Lambda service will "freeze" the nodejs process and the connection may not be closed until the process is "defrosted" by a new request.

That being said, opening/closing a connection with every request is an expensive thing to do. You should use AWS RDS Proxy to mitigate the cost opening/closing connections with every request.

@pankajtiwary
Copy link

@pankajtiwary pankajtiwary commented Mar 10, 2021

Dear All, I am using Sequelize 5.22.3 and have found very strange problem. I am also using RDS Postgres as database.
Whenever, I add connection pool setting in my sequelize configuration, It opens of hundreds of connections and that goes till 3000 connections. I have read that, there is some bug in connection pool which was later resolved.
I am just wondering if anyone has got this earlier and any solution or work around?
Thanks in Advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet