vinissimus / async-asgi-testclient

A framework-agnostic library for testing ASGI web applications

MIT License

159 stars 20 forks source link

Maintain hard references to tasks to prevent garbage collection #35

Closed MatthewScholefield closed 3 years ago

MatthewScholefield commented 3 years ago

This fixes issues with asyncio and garbage collection. Specifically, if a running task within asyncio doesn't have a hard reference, it can be garbage collected causing a number of strange errors like Task was destroyed but it is pending!. Holding a hard reference to running tasks solves this issue which is what this PR does.

To consistently replicate this error we can force garbage collection in between every step of the event loop as follows:

Edit /usr/lib/python3.*/asyncio/base_events.py and within def _run_once, near the bottom immediately within the for i in range(ntodo):, add import gc; gc.collect()
Make sure your testing framework is using asyncio (rather than uvloop for example) so that it runs the code we just modified (pytest does by default)

masipcat commented 3 years ago

Thank you for the PR I didn't know an unreferenced task could be garbage-collected.

Just one question: should we set to None the variables that keep the reference on __aexit__ (to prevent some kind of circular reference that leaks the TestClients)?

codecov-io commented 3 years ago

Codecov Report

Merging #35 (8d7f657) into master (d1b93fa) will increase coverage by 0.15%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   87.25%   87.40%   +0.15%     
==========================================
  Files           6        6              
  Lines         400      405       +5     
==========================================
+ Hits          349      354       +5     
  Misses         51       51

Impacted Files	Coverage Δ
async_asgi_testclient/testing.py	`88.73% <100.00%> (+0.24%)`	:arrow_up:
async_asgi_testclient/websocket.py	`74.11% <100.00%> (+0.62%)`	:arrow_up:

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 d1b93fa...8d7f657. Read the comment docs.

MatthewScholefield commented 3 years ago

Just one question: should we set to None the variables that keep the reference on __aexit__ (to prevent some kind of circular reference that leaks the TestClients)?

Good point. I don't think circular references are a problem for the most part in Python (although there are some edge cases), but setting them to None there definitely makes sense just from a logical perspective. I've pushed a change to implement that.

masipcat commented 3 years ago

Thank you very much. v1.4.6 released