coopernurse / node-pool

Generic resource pooling for node.js
2.37k stars 259 forks source link

`available` returns a non-0 after a `clear` operation #217

Open clark-pan opened 6 years ago

clark-pan commented 6 years ago

I have a situation where I'm calling clear without a drain before it. I'm logging out the size of the pool after the clear operation, and available still returns the amount of resources that was cleared. The resources themselves are destroyed, its just that pool still retains a reference to them through the private _availableObjects collection.

I want to see if we can remove them from the _availableObjects collection after a clear operation. I'm unsure if this is a use case that's the team would like to support.

Basically, calling clear, irregardless of whether or not you called drain first, would leave the _availableObjects queue intact. I saw the comments next to the clear method that its intended use case is to be used in conjunction with drain, so I appreciate that for all intended use cases, leaving these resources inside _availableObjects would have no appreciable difference, since you can no longer acquire new resources from the pool.

A bit of context for my use case, I'm using node-pool (through sequelize) running in an AWS Lambda environment. One of the things you need to be aware of when running in this environment is when AWS shuts down your instance and how it starts it up again. Basically it checks to see if there are any open handles still remaining in your execution environment, and only when the list is empty does it shut your Lambda instance down. For the next invocation of your Lambda function, AWS will start up the exact same execution environment, with all the resources in the state they were when they shut down the instance (whats called a 'warm boot'). They do this to speed up subsequent invocations of the same lambda function.

So in this execution model, node-pool's eviction process obviously plays havoc with your Lambda execution times (which you want to minimize to pay less cost/return a response to your user faster), so I've already disabled that. However, each of the underlying connections that sequelize opens to the database still counts as an open handle, which means you still have to close those connections.

sequelize does not keep a reference of all open connections that it makes, it delegates it to node-pool to track. So the method I've come up with to shutdown all open connections is to call clear on the underlying node-pool instance so that sequelize will properly destroy all open connections.

I've been logging the stats on the pool instance after I've ran clear on it and have noticed that available still returns a non-zero number (_allObjects is 0, so correspondingly size is 0. I have not set a minimum on the pool configuration).

Now, sequelize has implemented a validate function, so for all practical purposes, on the next 'warm boot', a new connection will be established and the system runs fine. However it just takes a minor bug in the validation process for this to create a bug.

I also understand that I could just create a new instance of sequelize for every run of the function, meaning I could run drain on the pool instance as well, but that slows the startup time of subsequence lambda functions because sequelize has to reinitialise all the model definitions.

I suspect the solution is a 1 liner in clear to remove all the resources in _availableObjects. I'm happy to raise a PR.

alanwu4321 commented 1 year ago

Is this solved? I am doing this after clear

for (let index = 0; index < this.pool._availableObjects.length; index++) {
                this.pool._availableObjects.shift()
            }

Should clear also destroy resources in ALLOCATE state?