Failed to release a connection with transaction started at 'aiomysql'

[Illarion Khlyestov]

Hello to all!

I meet strange bug that maybe cannot occure in real life, but it exist.

I've used example of transaction from aiomysql at some web server:

with (yield from engine) as conn:
    trans = yield from conn.begin()
    try:
        yield from conn.execute("insert into x (a, b) values (1, 2)")
    except Exception:
        yield from trans.rollback()
    else:
        yield from trans.commit()

But in case we have some interruption after transaction was started and short timeout(for example == 0.2)  on client side during request:

with (yield from engine) as conn:
    trans = yield from conn.begin()
    # note this sleep() call
    yield from asyncio.sleep(1)
    try:
        yield from conn.execute("insert into x (a, b) values (1, 2)")
    except Exception:
        yield from trans.rollback()
    else:
        yield from trans.commit()

server will receive concurrent.futures._base.CancelledError and will try to release a connection but it will fail due to transaction was opened:

Cannot release a connection with not finished transaction

Traceback (most recent call last):

  File "example_server.py", line 56, in create_tables

    yield from asyncio.sleep(1)

  File "/usr/local/lib/python3.5/asyncio/tasks.py", line 495, in sleep

    return (yield from future)

  File "/usr/local/lib/python3.5/asyncio/futures.py", line 385, in __iter__

    yield self  # This tells Task to wait for completion.

  File "/usr/local/lib/python3.5/asyncio/tasks.py", line 288, in _wakeup

    value = future.result()

  File "/usr/local/lib/python3.5/asyncio/futures.py", line 266, in result

    raise CancelledError

concurrent.futures._base.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "example_server.py", line 67, in create_tables

    yield from trans.commit()

  File "/home/legat/Projects/bugs_testing/aio_bugs/env/lib/python3.5/site-packages/aiomysql/sa/engine.py", line 189, in __exit__

    self._engine.release(self._conn)

  File "/home/legat/Projects/bugs_testing/aio_bugs/env/lib/python3.5/site-packages/aiomysql/sa/engine.py", line 117, in release

    raise InvalidRequestError("Cannot release a connection with "

aiomysql.sa.exc.InvalidRequestError: Cannot release a connection with not finished transaction

so at the next call to the same connection if we use aiomysql.sa.SAConnection it will not available(because it was not released).

Now I can handle this sittuation only with something like this:

with (yield from engine) as conn:
    try:
        trans = yield from conn.begin()
        try:
            yield from conn.execute("insert into x (a, b) values (1, 2)")
        except Exception:
            yield from trans.rollback()
        else:
            yield from trans.commit()
  except CancelledError:
      conn._transaction = None

In this case connection will be released.

My libs:

aiohttp==0.19.0

aiomysql==0.0.5

chardet==2.3.0

PyMySQL==0.6.6

SQLAlchemy==1.0.9

and if you want to reproduce fast here is an example of standalone server side code - http://pastebin.com/YkrS9FQx

Can someone advise what can be done in this case?

Thank you in advance

 

[nick...@datarobot.com]

Hi Illarion,

could you test master version of aiomysql? looks like this issue was reported in https://github.com/aio-libs/aiomysql/issues/46 and fixed https://github.com/aio-libs/aiomysql/commit/44ff6017f6c8f01e8d246659a6fc019e68eaf32d . But have not released new version on pypi.

[nick...@datarobot.com]

I will release new version today at night.

On Monday, December 7, 2015 at 2:08:44 AM UTC+2, Illarion Khlyestov wrote:

[Illarion Khlyestov]

Hi Nick!

Yes I know regarding that issue(I report it :) and that it was fixed.

But It don't help the deal.

In taht isuue was a trouble that trans.rollback() not working correct a little bit.

But here trans.rollback() (and trans._rollback_impl()) not called at all.

I've look to the source and when we first call

trans = yield from conn.begin()

such code from aiomysql.sa.connection.SAConnection executed:

@asyncio.coroutine

def begin(self):

    if self._transaction is None:

        self._transaction = RootTransaction(self)

        yield from self._begin_impl()

    ...

so now conn._transaction is not None.

But when our server receive concurrent.futures._base.CancelledError it will just call aiomysql.sa.engine.Engine.release() method:

def release(self, conn):

    if conn.in_transaction:

        raise InvalidRequestError("Cannot release a connection with "

                                  "not finished transaction")

    ...

where conn.in_transaction is aiomysql.sa.connection.SAConnection property:

@property

def in_transaction(self):

    return self._transaction is not None and self._transaction.is_active

So generaly if we open transaction and after receive CancelledError and we still not inside try/except statement that may rollback transaction - connection that contain that transaction will be failed to release.

понедельник, 7 декабря 2015 г., 11:20:00 UTC+2 пользователь nick...@datarobot.com написал:

[Nickolai Novik]

Illarion,

I see you point, I am thinking of 2 possible solutions:

1) implement proper context manager with python 3.5 features (__aenter__ and __aexit__)

2) change API a bit, when we create transaction object we do not perform any IO so CanceledError would not be an issue, for instance:

with (yield from engine) as conn:

    trans = conn.transaction()
    try:
        yield from trans.begin()

[Illarion Khlyestov]

Nikolay,

This is interesting. I will try to implement something like this and if I will succeed I will provide pull request(but it will be not very fast).

Also as I see there some another trouble may occure regarding nested transaction if API will be changed:

trans = yield from conn.begin()   # outermost transaction

trans2 = yield from conn.begin()  # "nested"

yield from trans2.commit()        # does nothing

yield from trans.commit()         # actually commits

also because they relay on _is_asctive transaction attribute.

But I think tests will show if something will fail.

Thank you for your advice

[Nickolai Novik]

I have better and simpler idea.

self._transaction = RootTransaction(self)
yield from self._begin_impl()

This two lines should be

yield from self._begin_impl()

self._transaction = RootTransaction(self)

So in case of CancelledError, connection could return to the pool, but even if connection did moved to transaction state despite exception, connection pool will drop it.

https://github.com/aio-libs/aiomysql/blob/master/aiomysql/pool.py#L194-L198

[Illarion Khlyestov]

Connection will be released by Pool in any case of ordering of transactions, as I've just check. 

But the trouble is that pool.release()  method will not called due to this lines inside sa.engine.Engine

https://github.com/aio-libs/aiomysql/blob/master/aiomysql/sa/engine.py#L114-L118

So there is such case that pool can release a connection, but sa.engine at the same time raise an exception?

Maybe there was some reason to do such way?

It was interesting for why these lines of code were added - I've commented them out and run the test. Only one fail - https://github.com/aio-libs/aiomysql/blob/master/tests/sa/test_sa_engine.py#L105-L113 but there is no any explanation.

Maybe someone now why it was designed such way?

вторник, 8 декабря 2015 г., 10:53:53 UTC+2 пользователь Nickolai Novik написал:

[anti...@gmail.com]

Illarion,

I think this was added to prevent forgotten transaction commit/rollback calls, and I'd say this is right - transaction should be processed and rollback should be called explicitly in case it's expected.
I like solution with context manager, it's obvious API for user to wrap transaction and pythonic.

@Nikolai second solution is good workaround in the current case, but generally it would be great to have ability to be in conversation with client and do the transaction simultaneously (e.g. you're in websocket handler).