Codecov Report

Merging #12642 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12642       /-   ##
==========================================
- Coverage   96.32%   95.85%   -0.47%     
==========================================
  Files          95       92       -3     
  Lines        9261     9057     -204     
  Branches       90       90              
==========================================
- Hits         8921     8682     -239     
- Misses        323      358       35     
  Partials       17       17              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e144683...ee3f724. Read the comment docs.

Here are some of the issues that I found related to AWS Lambda and connection pools: #3895, #4938, #5441, #6026, #8087, #8468, #9242, #10439, #11021, #11394, #12245, #12363, #12364, #12633

I have updated the guide with a simpler solution and more in-depth details for people that may be interested in diving deeper (diff).

I have updated the guide fixing some typos and other minor clarifications (diff).

Man, you really dug deep into this...

Thanks for this document it's very useful.
I didn't understand what connection pool settings you recommend in the end?
Also if you are using RDS proxy are any of the pool settings relevant?

Hi @amitaymolko:

I didn't understand what connection pool settings you recommend in the end?

Also if you are using RDS proxy are any of the pool settings relevant?

Since connection pooling is not used across invocations, the settings are not as important as before. For most cases the default settings should be OK:

{
  "max": 5,
  "min": 0,
  "idle": 10000,
  "acquire": 60000,
  "evict": 1000,
}

However you can optimize on a few things:

• max: as far as I know, all sequelize operations issue at most 2 concurrent queries. So you should probably not want to use a value lower than 2 if you don't want your queries to be serialized. If you explicitly issue queries concurrently, same principle applies. That being said be careful about not exceeding your underlying server's capacity.
• idle: this value is still important because your Lambda may timeout before you close the connections on the pool. So you want those connections to be closed when the function is invoked again. Use 1ms when you configure evict as recommended.
• acquire: since Lambda containers handle a single request at a time, you can use a more aggressive value here. If you have a max value for all the Lambda's current queries, then you can put a value that represents the maximum time you're willing to wait for a connection to be established (e.g. 3000).
• evict: this basically is the argument for a setTimeout() used by the pool to clean idle connections. Since you close them manually at the end of the request but your Lambda function may timeout, the ideal setting is to use the same value as your Lambda function's timeout.

I'll probably update the pull request with these recommendations.

@marcogrcr Really appreciate the time & thought put into this writeup!

I ran into the issue today when setting up a serverless framework API, and noted that the TL;DR workaround seemed to produce errors on warm lambda invocations (cold starts were fine):

Error: ConnectionManager.getConnection was called after the connection manager was closed!

I believe it's because calling connectionManager.close() rewrites connectionManager.getConnection to throw the error I was seeing on those warm invocations:

I got around this by writing a function to do the same things, but without rewriting getConnection,

  const closeConnection = async () => {
    await sequelize.connectionManager.pool.drain()
    return sequelize.connectionManager.pool.destroyAllNow()
  }

and calling it at the end of each handler in lieu of sequelize.connectionManager.close()

My understanding is that this setup closes & then reinstantiates the pool on each subsequent lambda invocation, which is all that we need-- and that reusing the connectionManager itself poses no problem given that setup.

If anyone reading this sees a flaw with that approach, or something I may have misunderstood with the original writeup, please let me know!

edit: this is using sequelize 6.3.5

@johnwquarles I can confirm your code works for me, thanks!
Sequelize: 5.21.2

@johnwquarles you're right about that. As a disclaimer, I have not had the opportunity run the TL;DR code on my project yet. When I analyzed that code, I got confused for a moment and thought the sequelize.connectionManager.pool.acquire() method was the one being overwritten instead of sequelize.connectionManager.getConnection(). My initial thought was that by using sequelize.connectionManager.initPools() the method overwrite would be reverted since a new instance of the pool would be created and assigned to sequelize.connectionManager.pool.

The code that you're proposing should do the trick since it's the equivalent of sequelize.connectionManager.close() except for the method override:

v6.3.5

v5.22.3

The only downside I see with this code is that sequelize.connectionManager.pool is not part of the public API:

v6.3.5

v5.22.3

This means that:

• TypeScript users will have to break type-safety.
• The code may break on newer versions of sequelize.

As an alternative to accessing internals, you can:

const { Sequelize } = require("sequelize");

let sequelize = null;

module.exports.handler = async function (event, callback) {
  // re-use the sequelize instance across invocations to improve performance
  if (!sequelize) {
    sequelize = await loadSequelize();
  } else {
    // restart connection pool to ensure connections are not re-used across invocations
    sequelize.connectionManager.initPools();
    // restore `getConnection` if it has been overwritten
    if (sequelize.connectionManager.hasOwnProperty("getConnection")) {
      delete sequelize.connectionManager.getConnection;
    }
  }

  try {
    return await doSomethingWithSequelize(sequelize);
  } finally {
    // close any opened connections during the invocation
    // this will wait for any in-progress queries to finish before closing the connections
    await sequelize.connectionManager.close();
  }
};

I guess the ideal solution would be to have sequelize.connectionManager.initPools() restores the overwritten method so there's no need to access class internals and avoid creating a new instance of Sequelize on every invocation. I guess it's a matter of proposing it to the sequelize team in a pull request to see if they accept it.

@amitaymolko / @johnwquarles : Thanks to your feedback, I have created a new revision (see diff here).

Hello everyone!! Thank you so much @marcogrcr for the detailed explanation! I will not merge yet because I want to take a closer look first, and I haven't had much time, please wait a little more and thank you very much!! 🚀

Thank you @cathykc! I've rebased the commit so that the CI build passes.

I don't think there's more than I can do other than wait for @papb to merge it.

Hello @papb

Just checking if you've had a chance to take a look at this pull request? It seems like various people are taking notice of it and many more would benefit from this being published in https://sequelize.org/.

Would you be able to share an ETA of when do you think someone from the sequelize project may be able to review?

Hey @papb just pinging here again to see if can find a chance to look at this PR. I understand that you're probably busy with other priorities, but it's been more than a year since your update 😬

Hey there :)

First of all: Thanks a bunch for your contribution to Sequelize! Much appreciated!
Second: Unfortunately we haven't had the chance to look into this ever since you created it. Sorry for that!

A couple of months ago, we have switched from master to main branch as our primary development branch and hence this PR is now outdated :(

If you still think this change is making sense, please consider recreating the PR against main. Thanks in advance and sorry for the additional work.

✌️

Hey @sdepold!

I have updated the pull request branch and rebased the commit as requested.

Hey @sdepold:

Can we re-open this pull-request? I rebased the branch on top of main as requested.

Haven't tried it but you have my trust :)

🎉 This PR is included in version 6.12.0-beta.1 🎉

The release is available on:

• npm package (@v6-beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

🎉 This PR is included in version 6.12.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

Hello @marcogrcr

"Since connection pooling is not used across invocations, the settings are not as important as before. For most cases the default settings should be OK"

Here (#4938 (comment)) you also wrote: "Keep in mind that AWS has released a product for AWS Lambda called Amazon RDS Proxy where you don't have to worry about connection pooling and you can create a new connection for every request."

How do you suggest we build our "lambda.handler"?

Followed the guide here (https://sequelize.org/docs/v6/other-topics/aws-lambda/) but we continue to experience "SequelizeConnectionAcquireTimeoutError" from time to time. I even removed the close() and initPool() because seems unnecessary by your comments.

And RDS connections to the DB seems to be stacking to the limit of out current allowance.

Thank you for the time you put doing this.

Hi all, thanks for all the work here.

I'm still having an issue with sequelize (^6.19.0) and lambda (nodejs12.x) w/ RDS Aurora Postgres.

We've implemented the code as described in the lambda doc, and are now seeing new error when a lambda function is invoked after "re-connecting" using the same sequelize instance (e.g. after calling the sequelize.connectionManager.initPools()).

The symptoms of the error:

• We're getting an error SequelizeDatabaseError with message Error: Connection terminated.

  • Here's a copy/paste of stack trace:
  • { name: 'SequelizeDatabaseError', parent: Error: Connection terminated at Connection.<anonymous> (/var/task/node_modules/pg/lib/client.js:131:36) at Object.onceWrapper (events.js:420:28) at Connection.emit (events.js:326:22) at Connection.EventEmitter.emit (domain.js:483:12) at Socket.<anonymous> (/var/task/node_modules/pg/lib/connection.js:62:12) at Socket.emit (events.js:314:20) at Socket.EventEmitter.emit (domain.js:483:12) at TCP.<anonymous> (net.js:675:12)

• This often happens in the middle of a request, after other successful DB calls. It seems that some connections in the pool work just fine, but others do not.

• We don't see this error in requests that have to initialize a brand new sequelize instance

• We also didn't see this error when we were closing and reconnecting the pool on each request (but clearly don't want to do this)

For more context, our entire API is implemented using a single lambda function, so we're reusing containers at a pretty high frequency.

Also, here's our sequelize config. I have played with many configurations (including using the config in the doc), and have not found any solutions, so I currently have it stripped down. Anything here that could be the problem?

	sequelize = new Sequelize(
		db_name,
		db_user,
		db_pass,
		{
			host: db_host,
			dialect: 'postgres',
			port: db_port,
			benchmark: true,
			minifyAliases: true,
		},
	);

TIA!