Skip to content

Conversation

@TheThing
Copy link
Contributor

During pool.open() it calls grow() and if grow fails, the open fails. Which is all fine and dandy.

However because await Promise.all(toPromise) threw an exception, the pool ends up in an incorrect state of having misnumbered pendingCreates totals since the failed and closed and non-existing connections are still being marked as pending.

This further causes problems as future calls to .open() might actually succeeds even though zero connections are open causing the entire pool to enter invalid state and never grow or work again because of this little code:

if (res.length === 0) {
  return
}

This patch fixes those problems as opened connections get added to the pool and failed connections don't get added to the pool.

@TheThing TheThing changed the title Fix connection errors preventing pool from being re-opened at later time Fix connection errors preventing the pool from ever being opened again Apr 27, 2022
@TheThing
Copy link
Contributor Author

I ended up accidentally rewriting the entire function almost D:
Sorry, once I get started at programming I can't stop myself

@TimelordUK
Copy link
Owner

Looks good thanks a lot. Will test and pull shortly. Appreciate the fix and time spent on this.

…accidentally refactored the entire function.
@TheThing
Copy link
Contributor Author

TheThing commented Apr 27, 2022

I wanted to write unit tests for this but it's hard to test timeout on connection because I didn't see any way to force it to timeout faster if that makes sense :b and I didn't wanna delay the unit tests for minutes because of one tiny problem.

I also squashed all five commits into one nice commit so as to keep the history and blame clean.

@TheThing
Copy link
Contributor Author

TheThing commented Apr 27, 2022

Unrelated, Uhh, is it okay if I accidentally do a PR at some point where I end up rewriting the entire or many parts of the pool driver? :>

@TimelordUK TimelordUK merged commit 73b63f5 into TimelordUK:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants