Fix "Unable to detect disconnect when using NOTIFY/LISTEN", Closes #249

[gjcarneiro]

What do these changes do?

When connection to DB is closed, any tasks pending on await connection.notifies.get() receives an exception, instead of hanging forever.

Are there changes in behavior for the user?

Now connection.notifies.get() can raise a psycopg2 exception, whereas before this would hang forever.

Related issue number

#249

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> (e.g. 588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the PR
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

[codecov]

Codecov Report

Merging #559 (289562f) into master (40efd3a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 289562f differs from pull request most recent head 6cc11e5. Consider uploading reports for the commit 6cc11e5 to get more accurate results

@@            Coverage Diff             @@
##           master     #559      +/-   ##
==========================================
+ Coverage   93.24%   93.36%   +0.12%     
==========================================
  Files          11       11              
  Lines        1525     1553      +28     
  Branches      179      182       +3     
==========================================
+ Hits         1422     1450      +28     
  Misses         72       72              
  Partials       31       31              

Impacted Files	Coverage Δ
aiopg/connection.py	95.82% <100.00%> (+0.01%)	⬆️
aiopg/utils.py	90.21% <100.00%> (+3.85%)	⬆️

---

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 40efd3a...6cc11e5. Read the comment docs.

[gjcarneiro]

What is this checklist item "Add a new news fragment into the CHANGES folder"? There is no CHANGES folder, as far as I can see...

[vir-mir]

ignore, I will correct later

[michaelbeaumont]

@vir-mir hi! any news on this PR?

[Pliner]

@vir-mir I'm interested in the fix too. Could you review it, please? 😉

[albertodonato]

any news on this PR? I would be interested in the fix too.

@gjcarneiro it seems there are a few conflicts, could you please update the branch?

[gjcarneiro]

I've updated the PR for current master. Can we get this merged please? Been 2 years... :/

[Pliner]

Hi @gjcarneiro,

Sorry for the so long delay. I hope I will be able to review it later today or tomorrow. Thanks for rebasing it 👍

[Pliner]

@gjcarneiro I'm a bit concerned about one aspect of the implementation: it covers only get method of the queue(I suspect not so many people use get_no_wait(for instance), but anyway).

What do you think about exposing a kind of proxy object which exposes only async get method(probably we need to expose any other methods/properties)?

[gjcarneiro]

@gjcarneiro I'm a bit concerned about one aspect of the implementation: it covers only get method of the queue(I suspect not so many people use get_no_wait(for instance), but anyway).

That's a good point. I didn't think about get_nowait(), it would probably also need to check for the psycopg exception, for consistency.

What do you think about exposing a kind of proxy object which exposes only async get method(probably we need to expose any other methods/properties)?

Well, I tried to make this 100% backwards compatible. What you propose, I don't know if it would be 100% compatible? It would probably be cleaner, but... I'll think about it.

[gjcarneiro]

I've updated the PR with making ClosableQueue a proxy class instead of a subclass.

[gjcarneiro]

I've made a new version. Any more comments?

[gjcarneiro]

ping

[Pliner]

Sorry for the delay, quite overloaded at work, hope will be able review it on this week.

[albertodonato]

Awesome! Could you please make a release with this fix?

[Pliner]

Awesome! Could you please make a release with this fix?

Hi @albertodonato,

Surething, but it will be the beta version(probably 1.3.0b4) to double-check that everything works. I'm going to deploy this release later today.

[Pliner]

https://pypi.org/project/aiopg/1.3.0b4/