search

myndzi / pool2

38 stars 8 forks source link

Fix issue with resources getting lost when all requests 'fulfilled' #20

Closed smozely closed 8 years ago

smozely commented 8 years ago

This issue would occur if there was a database slowdown that was long enough that all outstanding requests hit the requestTimeout, then when the DB comes back, a resource would get lost.

_maybeAllocateResource was pulling an available resources out of the queue before checking if any requests did not already have 'fulfilled' set.

This was causing that resource to be lost as it was not added back to the queue. This fix just pulls it from the queue after making sure there is a request that still needs to be handled.

There might be the same issue if a request is fulfilled while a resource ping is happening.

myndzi commented 8 years ago

I'm looking into this. The code fix seems obvious but I'm not happy about the test, and while investigating more thoroughly I ran into a weird problem where the tests are behaving differently when debug is enabled. Thanks for pointing it out!

myndzi commented 8 years ago

Okay! Debug was making things slow enough for a timing error to show up, allowing me to fix a bug with a synchronous throw in the ResourceRequest constructor. Also, you were correct about this error happening in the ping function as well, that has also been fixed. Overall, I should have considered the effects of adding the request timeout feature more thoroughly; it's hard when I have spent months without thinking about this code. It was designed without such a thing, so some assumptions I made are biting me.

Overall, I maybe should have used promises after all, though I originally designed this to be light on the dependencies; some of the async logic gets pretty ugly when it's waiting on two separate asynchronous results that can both fail. I'm closing this PR due to a more complete fix, but thank you for your contribution!

smozely commented 8 years ago

Sweet Thanks!