Issue: Connection pooling: re-running failed query in sql_error_handler
Author: tuukkamustonen
State: closed
Created: 2015-05-13 07:32:55

Restructued from my email at https://groups.google.com/forum/#!topic/peewee-orm/LDuaQtT-ZTc

I find connection pooling somewhat unusable at the moment due to following:

When using `playhouse.pool.PooledMySQLDatabase`, if database is rebooted (non-HA), open connections in the pool fail the next time they are used.

Those failed connections can be closed by:

```
class ConnectionClosingMySQLDatabase(playhouse.pool.PooledMySQLDatabase):

    CONN_FAILURE_CODES = [
        2006,  # MySQL server has gone away (error(32, 'Broken pipe'))
        2013,  # Lost connection to MySQL server during query
    ]

    def sql_error_handler(self, exception, sql, params, require_commit):

        if isinstance(exception, pymysql.err.OperationalError):

            if exception.args[0] in self.CONN_FAILURE_CODES:
                db.manual_close()

        return True
```

However, after database has become online again, the first query for each connection will fail, as this logic only closes the broken connection.

(Also, it seemed like each open connection would break for DB going down temporarily, even if that connection was not used during the downtime.)

In order to also re-run the query:

```
class ReconnectingMySQLDatabase(playhouse.pool.PooledMySQLDatabase):

    CONN_FAILURE_CODES = [
        2006,
        2013,
    ]

    def sql_error_handler(self, exception, sql, params, require_commit):

        if isinstance(exception, pymysql.err.OperationalError):

            if exception.args[0] in self.CONN_FAILURE_CODES:
                db.manual_close()
                db.connect()

                db.execute_sql(sql, params or (), require_commit=require_commit)

                return False

        return True
```

But this gets:

```
  ...
  File "/home/musttu/Code/virtualenvs/cs/local/lib/python2.7/site-packages/pymysql/cursors.py", line 71, in _check_executed
    raise ProgrammingError("execute() first")
ProgrammingError: execute() first
```

I believe this happens because `Database.execute_sql` returns the original `cursor` and not the cursor of the fixed connection:

```
    ....
        cursor = self.get_cursor()
        try:
            cursor.execute(sql, params or ())
        except Exception as exc:
            if self.get_autocommit() and self.autorollback:
                self.rollback()
            if self.sql_error_handler(exc, sql, params, require_commit):
                raise
        else:
            if require_commit and self.get_autocommit():
                self.commit()
    return cursor
```

If I could fetch and return a new cursor from `sql_error_handler` (now it returns bool), I assume it could work?


Have you tried telling the pool to close all connections in the error handler? This might work better:

``` python
    def sql_error_handler(self, exception, sql, params, require_commit):

        if isinstance(exception, pymysql.err.OperationalError):

            if exception.args[0] in self.CONN_FAILURE_CODES:
                db.close_all()  # Changed this line from `manual_close()`.

        return True
```


I didn't try that, but while it would "heal" all the other connections, this one request would still fail (per process). It might not sound much, but...

I also wonder: would `close_all()` really close all of the connections, even those in active use at the point of time in other threads, causing them to fail their requests?

Well, I assume it doesn't matter as they would fail anyway (as the connections are broken).


> I also wonder: would close_all() really close all of the connections, even those in active use at the point of time in other threads, causing them to fail their requests?

No, those connections would not be closed and presumably they'd fail with the same `OperationalError`.


I think the `close_all()` solution is as good as it gets, so I'm going to close this for now.


@coleifer So, it's not possible to allow returning a new cursor? Would it be too complex, technically?


Yeah, returning the new cursor is the crux of the issue. I'm not sure of the cleanest way to do that with `sql_error_handler()`. I'd prefer not to break existing implementations, but at the same time I think it's failing to do what I had designed it for in the first place.

I think I'll see about reworking the method.


I'm not positive of all the conditions that might cause an `OperationalError`, but I can imagine a max-recursion issue if the `connect()` after `close()` succeeded, and the subsequent `execute_sql()` raise an `OperationalError`:
- `execute_sql(...)`
- `OperationalError` occurs
- call `sql_error_handler()`
  - `db.close()`
  - `db.connect()` - let's assume this succeeds
  - `db.execute_sql(...)`  - retry the query
  - `OperationalError` occurs
  - call `sql_error_handler()` ...

You get the drift... If the issue was the db being unavailable this wouldn't be the case as the `connect()` would fail, but perhaps there are other reasons an `OperationalError` might be raised?


The more I think about it, the more I'm thinking that if the query fails with an `OperationalError`, it probably doesn't make sense to immediately retry. **Unless** you're using a connection pool, and there's a possibility the connection you just checked out is stale and all you need to do is reconnect to be good to go... Hmm...


I agree, this sort of re-connection logic makes sense only when using connection pooling. And even then, finding out all the situations where a re-connection might help (and not cause side effects) could be tricky... or not. I'm no expert with MySQL - the two failure codes (`2006`, `2013`) were just my first findings when I bumped into this issue.

Regarding stale connections, I was in the assumption that `connect()` (which I'm calling for each requests) actually ensures that connection is not stale (and closes + opens a new one if it is). Or does that logic apply to just `stale_timeout`?

I didn't realize the recursion issue. Wouldn't it be possible to catch that with a counter (or simply allow single retry)?

Then, after thinking about this more, I don't think this is as critical as I initially thought. I mean, if the database is down for a moment, the milk is already spoiled, some requests will fail anyway. It doesn't make big difference if couple of requests fail even after DB is back up. `close_all()` helps a lot.

(Though I would wish that was built-in to the pool somehow.)

The most annoying scenario is when DB temporarily goes away and comes back without any requests being processed between. As in my original post: _each open connection would break for DB going down temporarily, even if those connections were not used during the downtime._ That would mean a couple of failing requests, where in perfect world none would fail. I assume it's not that bad, either.


> Regarding stale connections, I was in the assumption that connect() (which I'm calling for each requests) actually ensures that connection is not stale (and closes + opens a new one if it is). Or does that logic apply to just stale_timeout?

When a connection is checked out, it first checks if the connection is stale, and if not, it will check if the connection "is closed" by calling the `_is_closed()` method. The MySQL implementation looks like this:

``` python
    def _is_closed(self, key, conn):
        is_closed = super(PooledMySQLDatabase, self)._is_closed(key, conn)
        if not is_closed:
            if hasattr(conn, 'open'):
                # MySQLdb `ping()` seems to always return `None` in my testing.
                # So the `open` attribute will be used instead.
                is_closed = not bool(conn.open)
            else:
                # pymysql `ping([reconnect=True])` will indicate if the conn
                # is closed or not.
                try:
                    is_closed = not conn.ping(False)
                except:
                    is_closed = True
        return is_closed
```

As to the case where the connection goes away then comes back, I would have thought the logic in `_is_closed()` would catch those. Maybe something isn't working as I had thought?


What I did was simply restart MySQL (an AWS RDS instance) and throw in requests after it was back up. It was a master-standby setup, but I didn't really activate failover to standy - it was the same master going down and back up again. Of course, there's always also possibility of AWS RDS doing interesting things compared to simpler, local MySQL installation. Give me couple of days and I'll verify the behavior when I'm back at the office.

> As to the case where the connection goes away then comes back, I would have thought the logic in _is_closed() would catch those. Maybe something isn't working as I had thought?

Looking at pymysql's `ping()`, if `socket` exists, `pymysql` will ping the DB, and if that doesn't work, exception is thrown and peewee's `_is_closed()` will return `True` as expected.

What makes me wonder, is pymysql's `Connection.open`:

```
@property
def open(self):
    return self.socket is not None
```

As peewee relies solely on this (when available), maybe in some scenarios the socket _exists_, but it's actually in a failed state and actual operations will fail? Is `open` proper way to check the health of the connection?


Ok I tested it locally:
- start app
- make some requests to fill the connection pool
- stop mysql
- start mysql (no requests during downtime)
- make a request

The result is:

```
  File "/home/musttu/Code/virtualenvs/cs/local/lib/python2.7/site-packages/pymysql/connections.py", line 916, in _write_bytes
    raise err.OperationalError(2006, "MySQL server has gone away (%r)" % (e,))
```

I took a look at pymysql's `Connection.open` and indeed it returns `True` even after mysql restart. So I assume socket does not get closed, because it is not explicitly instructed to be closed by the driver.

A quick googling gave me: http://stackoverflow.com/questions/3335342/how-to-check-if-a-mysql-connection-is-closed-in-python which actually states the same.

Unfortunately, it doesn't sound like there is a way to directly know if the connection is functional or not. The expert there suggests a try/except block, which to me sounds like either failing that single request and callling `close_all()` (to play safe) or doing some sort of a retry logic.

Of course, if pool always pinged the database, broken connections would be caught, but yeah that would also introduce latency, waste IO and such.


Thanks for looking into it some more. Well, I guess there's not much to do here. If you wanted to subclass the pool and modify the `_is_closed()` method to PING that'd be one option.


I agree, this is a corner case anyway and `close_all()` is good enough. Thanks for reviewing it.


In case you're interested, I've got a mixin for retrying failed queries in 017e4e4952f0b429dd21f0e90465a0f644cf6015 . Docs:

http://docs.peewee-orm.com/en/latest/peewee/database.html#automatic-reconnect