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.

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

Remove blocking connection.cancel() method #570

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

brianmaissy
Copy link
Contributor

Closes #491
Closes #375

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2019

This pull request fixes 1 alert when merging 1d0cdde into 9fdf7b9 - view on LGTM.com

fixed alerts:

  • 1 for Use of the return value of a procedure

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2019

This pull request fixes 1 alert when merging b7fe8e6 into 9fdf7b9 - view on LGTM.com

fixed alerts:

  • 1 for Use of the return value of a procedure

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #570 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
- Coverage   94.35%   94.05%   -0.31%     
==========================================
  Files          27       27              
  Lines        3740     3700      -40     
  Branches      171      169       -2     
==========================================
- Hits         3529     3480      -49     
- Misses        179      189      +10     
+ Partials       32       31       -1
Impacted Files Coverage Δ
aiopg/utils.py 81.53% <ø> (-2.31%) ⬇️
tests/test_pool.py 99.75% <100%> (ø) ⬆️
tests/test_connection.py 99% <100%> (+0.36%) ⬆️
tests/pep492/test_async_await.py 96.11% <0%> (-3.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fdf7b9...953e6e7. Read the comment docs.

@brianmaissy brianmaissy force-pushed the bugfix/depecate_cancel branch from b7fe8e6 to 953e6e7 Compare June 4, 2019 13:21
@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2019

This pull request fixes 1 alert when merging 953e6e7 into 9fdf7b9 - view on LGTM.com

fixed alerts:

  • 1 for Use of the return value of a procedure

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #570 (a485878) into master (b5d3490) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
+ Coverage   92.89%   93.07%   +0.18%     
==========================================
  Files          13       13              
  Lines        1562     1531      -31     
  Branches      179      172       -7     
==========================================
- Hits         1451     1425      -26     
+ Misses         79       78       -1     
+ Partials       32       28       -4     
Impacted Files Coverage Δ
aiopg/connection.py 91.86% <100.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5d3490...a485878. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2019

This pull request fixes 1 alert when merging 08c618b into 9fdf7b9 - view on LGTM.com

fixed alerts:

  • 1 for Use of the return value of a procedure

@CLAassistant

This comment has been minimized.

@brianmaissy
Copy link
Contributor Author

Is this going to actually be merged if I go through the CLA process?

@Pliner
Copy link
Member

Pliner commented Dec 11, 2020

Hi @brianmaissy,

First of all, thanks for your contribution and sorry for the huge delay in the reply.

Do you have time to rebase this PR? It would be nice to include it to the next minor version.

@brianmaissy brianmaissy force-pushed the bugfix/depecate_cancel branch from 08c618b to a485878 Compare December 13, 2020 08:08
@Pliner
Copy link
Member

Pliner commented Dec 15, 2020

@brianmaissy Thanks for rebasing, I've missed it.

@Pliner
Copy link
Member

Pliner commented Dec 15, 2020

@asvetlov Are you ok with this PR?

I'm going to merge and release it as 1.2beta if so.

@Pliner Pliner changed the title remove blocking connection.cancel() method Remove blocking connection.cancel() method because it's unsupported in psycopg Dec 15, 2020
@Pliner Pliner changed the title Remove blocking connection.cancel() method because it's unsupported in psycopg Remove blocking connection.cancel() method because Dec 15, 2020
@Pliner Pliner changed the title Remove blocking connection.cancel() method because Remove blocking connection.cancel() method Dec 15, 2020
@Pliner
Copy link
Member

Pliner commented Dec 16, 2020

Well, let's merge it and test. @asvetlov any later suggestions are welcome

@brianmaissy Will you be able to test it on your workload?

@Pliner Pliner merged commit 04a8b45 into aio-libs:master Dec 16, 2020
@brianmaissy
Copy link
Contributor Author

brianmaissy commented Dec 16, 2020

Yes. Are you going to release a beta build?

@brianmaissy brianmaissy deleted the bugfix/depecate_cancel branch December 16, 2020 08:52
@Pliner
Copy link
Member

Pliner commented Dec 16, 2020

Yes. Are you going to release a beta build?

Thanks. https://pypi.org/project/aiopg/1.2.0b1/

@brianmaissy
Copy link
Contributor Author

seems to be working fine 👍

@Pliner
Copy link
Member

Pliner commented Dec 20, 2020

seems to be working fine 👍

Thanks 🔥

I'm going to test it during the next week as well.

@brianmaissy
Copy link
Contributor Author

@Pliner I think I found what may be a problem: if a TimeoutError is raised while waiting for a query while in a transaction context manager, it used to cancel the query, and then run the ROLLBACK. Now it closes the connection, and when it tries to run the ROLLBACK it throws a psycopg2.InterfaceError: connection already closed exception. I have a test which replicates the problem, should I open a new ticket and PR?

@Pliner
Copy link
Member

Pliner commented Dec 23, 2020

@Pliner I think I found what may be a problem: if a TimeoutError is raised while waiting for a query while in a transaction context manager, it used to cancel the query, and then run the ROLLBACK. Now it closes the connection, and when it tries to run the ROLLBACK it throws a psycopg2.InterfaceError: connection already closed exception.

@brianmaissy I have the same trouble 👍

I have a test which replicates the problem, should I open a new ticket and PR?

Yes, please 😊

brianmaissy added a commit to brianmaissy/aiopg that referenced this pull request Dec 23, 2020
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 added a commit to brianmaissy/aiopg that referenced this pull request Dec 23, 2020
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 added a commit to brianmaissy/aiopg that referenced this pull request Dec 23, 2020
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 added a commit that referenced this pull request Mar 22, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connection.cancel can hang Aiopg blocks event loop during query cancel
3 participants