search

coopernurse / node-pool

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

Add destroy timeout to prevent pool.clear from hanging when destroy promise never resolves #288

Closed cressie176 closed 3 years ago

cressie176 commented 3 years ago

Hello, and thank you for this library. We've been using it in rascal for managing RabbitMQ channel pool and it's proved very helpful. We recently had a bug reported which is partially caused because under certain error conditions closing a channel never yields. We do this via the factory destroy override and consequently its promise never resolves. I've fixed this using a timeout, but was wondering what you think about adding a configuration option destroyTimeoutMillis which would work in a similar way to acquireTimeoutMillis? We would be happy to work on this feature, but conscious that you may not have the time to review, merge and release. Let me know.

Kind regards,

Steve

Kikobeats commented 3 years ago

Hey, thanks for reporting. I will happy to accept a PR for this 🙂

What could be a good destroyTimeoutMillis default value? I guess you want to be sure resources are gracefully cleaned after a while, but not sure what should be the value there for cover most of the common situations

cressie176 commented 3 years ago

Hi @Kikobeats,

Thanks for the prompt reply. How about defaulting to no limit to maintain backwards compatibility and to keep it consistent with acquireTimeoutMillis?

Kikobeats commented 3 years ago

I think it's a good start, yes!

cressie176 commented 3 years ago

Added via https://github.com/coopernurse/node-pool/pull/289. Thank you