Query Promise does not resolve (again) about mysql

[kelunik]

@brstgt It could make use of https://github.com/kelunik/retry, which already has an interface for backoffs.

[brstgt]

If that helps, i enabled MYSQL_DEBUG. Here is the output along with my own logs:
https://gist.github.com/brstgt/0e808d83908581e7fdad669eb3571f38

After I send the last query (SELECT * FROM ...) MYSQL_DEBUG doesn't output anything any more. Seems like it's never written due to some write barrier with a race condition?!

[brstgt]

After trying different cases for a while it seems it happens if the connection is closed due to wait timeout

[trowski]

Can you try again with the latest master? I recently added a feature in pools that removes idle connections after a given timeout (default is 60s). I also fixed (I think…) making queries on dead connections so they fail a few commits ago, perhaps you didn't get those yet.

[brstgt]

It solves the problem that the promise is not resolved but it raises an error: "The connection has been closed" even though the pool could handle that situation.
I never query the connection directly, I always query the pool.

IMHO you need an observer for connection state changes so the pool can react on that

[brstgt]

Another observation: When you close idle connections you have to handle if a connection has been already closed by the server. Otherwise it throws a Error with 'The connection has been closed' in Pool::$timeoutWatcher

[trowski]

Merged your PR and fiddled with the Pool implementation so it checks that the connection is still alive both when a connection goes back into the pool and when it is coming out.

[kelunik]

@trowski I guess queries should be retried if the connection goes away before the write could complete.

[brstgt]

@trowski Can we change the Error when writing to a closed connection to a suitable Exception like ConnectionClosedException? It's better for error handling outside of the connection

@kelunik Retrying is great but only if you are not inside of a transaction as this would change the state / context of that query. Maybe it's safer to handle that outside of the connection?

[trowski]

@brstgt Sure, it could be changed to a ConnectionException or a new ConnectionClosedException. However I'm bothered by the connections being unexpectedly closed? What's happening there? Is the timeout on the server really short or is some error occurring?

[brstgt]

We have a wait_timeout of 10s to keep the number of idle connections low. In the past we had situations where php/mysqli didn't close the connection correctly in some situations which can lead to "Too many connections" errors. Closing the connections on server side when idle helps there.

Dealing with closed connection shouldn't be a big deal. On a "ClosedConnectionException" you can just retry or better - the pool manages this transparently if a connection is closed when being idle. But as already mentioned, a retry should only happen when not in a transaction because otherwise the whole transaction must be replayed.

[kelunik]

@brstgt If you're inside the pool, there shouldn't be any transaction, no? Because transactions check out the connection from the pool.

[trowski]

@brstgt I'm not sure if the pool should automatically retry queries… that will take a little more thought as to how that should be controlled, since there's obviously a few questions that go along with that: how many times should it be retried and under what circumstances.

The Pool implementation in the current master should not be returning closed connections at all. If it is, then there's still some issue. I'd also recommend using setIdleTimeout on the pool to set it to something less than the connection timeout… maybe as little as 5s. Also consider setting the wait_timeout to something a little higher now as you're the connections a bit differently now.

[brstgt]

By saying the pool can handle that transparently, I mean that if a connection is closed by the server (no matter for what reason) the connection detects that and the pool can listen for a disconnect event to remove that connection from the pool. Or at least, the pool should not elect a closed connection when fetching a connection for a query.

One option for retries in Pool::query() (when not in transaction, as the pool never handles transactions) there could be an optional param with a retrypolicy that takes retry-times and a backoff multiplicator (example: https://android.googlesource.com/platform/frameworks/volley/+/idea133/src/com/android/volley/DefaultRetryPolicy.java)

Adjusting timeouts can be seen as an optimization (to avoid unnecessary retries / connection cleanups) but should not be considered as a fix IMHO.

[brstgt]

P.S.: I can check if f077c40 works as expected next week. Until now, I dont have this commit in prod, so I check for Errors raised if a connection is closed and do a retry in my app code

[trowski]

@brstgt The pool should now handle connection disconnections transparently with f077c40. Looking forward to hearing how it goes.

[brstgt]

Works :)