Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't rollback a transaction if the connection is already closed #777

Closed
brianmaissy opened this issue Dec 23, 2020 · 1 comment · Fixed by #778
Closed

Don't rollback a transaction if the connection is already closed #777

brianmaissy opened this issue Dec 23, 2020 · 1 comment · Fixed by #778

Comments

@brianmaissy
Copy link
Contributor

Caused by #570, in which we close the connection in any situation where we would have cancelled the query, since we don't have an async way of cancelling the query.

One possible situation in which we could find ourselves closing the connection is in the case where a query timed out or was cancelled (in an asyncio sense). In that case, we now close the connection, but the calling code may be inside a transaction context manager, which will try to emit a ROLLBACK before exiting the block, since an exception was raised.

The net effect is that instead of getting a TimeoutError as excepted, the user gets a psycopg2.InterfaceError: connection already closed exception, which gets raised during the handling of the TimeoutError. In this case, since we were the ones who closed the connection, and we are also the ones trying to run the ROLLBACK, it doesn't seem fair to the user not to deal with it properly (as opposed to if a user runs some cleanup query after a TimeoutError was thrown, in which case it would be the user's responsibility to check that the connection is still open).

There are many possible ways to solve this, but I think the most elegant might be just not to run the ROLLBACK if the connection is closed. This is nice because it isn't just a workaround for a bug, semantically it makes sense. If the connection is closed, the transaction is over, and there's no need to run a ROLLBACK.
brianmaissy added a commit to brianmaissy/aiopg that referenced this issue Dec 23, 2020
@brianmaissy
Don't run ROLLBACK when the connection is closed.
f91ee33 
This can be caused when a query times out while running, for example,
and the connection is closed as a result (as opposed to cancelling the
query, since PR aio-libs#570). In this case, we would rather not emit the
ROLLBACK (the connection is already closed, so the transaction is over
anyway), rather than raising an exception when trying to use a
connection which is already closed.

See issue aio-libs#777.
@Pliner
Copy link
Member

Pliner commented Dec 23, 2020

There are many possible ways to solve this, but I think the most elegant might be just not to run the ROLLBACK if the connection is closed. This is nice because it isn't just a workaround for a bug, semantically it makes sense. If the connection is closed, the transaction is over, and there's no need to run a ROLLBACK.

I have merely the same thoughts about the fix.

brianmaissy added a commit to brianmaissy/aiopg that referenced this issue Dec 23, 2020
@brianmaissy
Don't run ROLLBACK when the connection is closed.
d576656 
This can be caused when a query times out while running, for example,
and the connection is closed as a result (as opposed to cancelling the
query, since PR aio-libs#570). In this case, we would rather not emit the
ROLLBACK (the connection is already closed, so the transaction is over
anyway), rather than raising an exception when trying to use a
connection which is already closed.

See issue aio-libs#777.
@brianmaissy brianmaissy mentioned this issue Dec 23, 2020
Merged
brianmaissy added a commit to brianmaissy/aiopg that referenced this issue Dec 23, 2020
@brianmaissy
Don't run ROLLBACK when the connection is closed.
5408cac 
This can be caused when a query times out while running, for example,
and the connection is closed as a result (as opposed to cancelling the
query, since PR aio-libs#570). In this case, we would rather not emit the
ROLLBACK (the connection is already closed, so the transaction is over
anyway), rather than raising an exception when trying to use a
connection which is already closed.

See issue aio-libs#777.
@Pliner Pliner mentioned this issue Mar 21, 2021
Merged
3 tasks
Pliner added a commit that referenced this issue Mar 22, 2021
@brianmaissy @Pliner
Don't run ROLLBACK when the connection is closed. (#778)
2e6e0f5 
* Don't run ROLLBACK when the connection is closed.

This can be caused when a query times out while running, for example,
and the connection is closed as a result (as opposed to cancelling the
query, since PR #570). In this case, we would rather not emit the
ROLLBACK (the connection is already closed, so the transaction is over
anyway), rather than raising an exception when trying to use a
connection which is already closed.

See issue #777.

* add tests for cancelled queries in transaction

* Update test_transaction.py

* Update test_sa_transaction.py

* Update connection.py

Co-authored-by: Yury Pliner <yury.pliner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't run ROLLBACK when the connection is closed. brianmaissy/aiopg
2 participants
@brianmaissy @Pliner