fix: pending connection handling

[mikeroelens]

Was looking through connection pool code and noticed a few things that may cause problems. I haven't proved them out in practice on impact but seems like without these fixes the pool could technically end up in a bad state

https://www.loom.com/share/563a05d796404378ac418580ba65038e

[changeset-bot]

🦋 Changeset detected

Latest commit: 0b15126

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages

Name	Type
slonik	Minor
@slonik/benchmark	Minor
@slonik/driver	Minor
@slonik/errors	Minor
@slonik/eslint-config	Minor
@slonik/pg-driver	Minor
@slonik/dataloaders	Minor
@slonik/sql-tag	Minor
@slonik/test-ssls	Minor
@slonik/types	Minor
@slonik/utilities	Minor
slonik-interceptor-field-name-transformation	Minor
slonik-interceptor-query-cache	Minor
slonik-interceptor-query-logging	Minor
slonik-sql-tag-raw	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mikeroelens]

pendingConnections.pop(); will remove the most recently added pending connection from the array. But by the time this code runs the last connection in the array might not be the same one that threw an error right?

E.g.

• Pending Connection A added. pendingConnections = [A]
• Pending Connection B added. pendingConnections = [A,B]
• 5s later A has timed out and throws an error. pendingConnections.pop() is called and now B is removed and we’re left with [A]. But it should've been A that was removed

[mikeroelens]

Was able to reproduce this behaviour with a test. Although it also required making this change so that we don't accidentally call splice with -1. This was somewhat hiding the failure because it was still removing an item from pendingConnections even though it wasn't the correct one.

[mikeroelens]

I moved this up further. If not technically we might call waitingClients.shift() but then never actually acquire a connection for that waiting client since we throw an error.

[mikeroelens]

This is causing some tests to fail because it's throwing Connection is not idle.. From logging it looks like the state is actually PENDING_DESTROY. Which I think indicates that this is a true problem in practice, we shouldn't be calling waitingClients.shift() if we can't actually do something with that waiting client.

Maybe we should change this to:

const onRelease = () => {
        if (connection.state() !== 'IDLE') {
            return;
        }

        ...

?

[mikeroelens]

Same thing here. We’re calling waitingClients.shift() but then if for some reason we’re below the minimum pool size we return early and the waiting client never gets a connection.

I removed the return after addConnection(), I don't see an obvious reason it means we need to return early but could be missing something

[gajus]

@mikeroelens any chance of getting this across the line?

[mikeroelens]

Have this as error level because it shouldn't happen if we don't have any bugs

[gajus]

This is amazing!