Test for ResourceWarning:unclosed transport #4713

spacemanspiff2007 · 2020-04-26T09:58:23Z

What do these changes do?

Add a test for ResourceWarning:unclosed transport

Are there changes in behavior for the user?

None

Related issue number

#4703

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> for example (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."

#4703

spacemanspiff2007 · 2020-04-26T11:30:28Z

loop = asyncio.get_event_loop()

is failing and I am not sure how to fix it.

webknjaz · 2020-04-26T11:36:28Z

tests/test_resource_warning.py

+
+
+def test_ressource_warning(warning_setup):
+    loop = asyncio.get_event_loop()


It's failing because it should be in async def to work. But then, why don't you just do await _test_warning()?

webknjaz · 2020-04-26T11:37:27Z

tests/test_resource_warning.py

+        if last_warning:
+            await client.close()
+            raise ResourceWarning(last_warning)


Why do you need all this complex logic here? Tests should be simple and not catch errors.

webknjaz · 2020-04-26T11:39:11Z

tests/test_resource_warning.py

+def warning_setup():
+    # setup warning
+    old_func = warnings.showwarning
+    warnings.showwarning = print_warnings
+    warnings.simplefilter('default')
+
+    yield
+
+    warnings.showwarning = old_func


This can be simplified as

Suggested change

def warning_setup():

# setup warning

old_func = warnings.showwarning

warnings.showwarning = print_warnings

warnings.simplefilter('default')

yield

warnings.showwarning = old_func

def warning_setup(monkeypatch):

monkeypatch.setattr(warnings, 'showwarning', print_warnings)

warnings.simplefilter('default')

tests/test_resource_warning.py

spacemanspiff2007 · 2020-04-26T15:16:36Z

I modified the test but when running through pytest the behavior is different.
The test does not fail and the warnings appear (at least it seems like it) once the test is finished.

2020-04-26 17:07:13.406819 | Status: 200, next request in 45s
2020-04-26 17:07:58.647407 | Status: 200, next request in 60s
2020-04-26 17:08:58.872852 | Status: 200, next request in 75s
2020-04-26 17:10:14.157158 | Status: 200, next request in 90s
2020-04-26 17:11:44.414320 | Status: 200, next request in 105s
2020-04-26 17:12:14.449038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C45B88>
2020-04-26 17:12:14.449038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C2D248>
2020-04-26 17:12:14.449038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C3CF08>
2020-04-26 17:12:14.449038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C485C8>
2020-04-26 17:12:14.449038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C454C8>
2020-04-26 17:12:14.450038 | C:\Progs\WinPython-64bit-3.7.4.0Ps2\python-3.7.4.amd64\lib\asyncio\sslproto.py:322 ResourceWarning:unclosed transport <asyncio.sslproto._SSLProtocolTransport object at 0x0000000004C3C5C8>

setup.cfg

webknjaz · 2020-04-26T21:29:30Z

@spacemanspiff2007 maybe revert the test to normal def function but instead of get_event_loop() use loop = asyncio.new_event_loop().

spacemanspiff2007 · 2020-04-27T03:37:38Z

Now it fails as expected

tests/test_resource_warning.py

webknjaz · 2020-04-27T10:25:26Z

tests/test_resource_warning.py

+            break
+
+        if now > next_request:
+            async with client.get('https://heise.de') as resp:


It's better to use a local server. Here's fixtures that will help you to set up a trusted TLS context while still using real-world SSL: c180800#diff-5d43d8b7848138232c802e32ab023dd3R271-R272

How would I use the fixture?

Add it as an arg into fixtures that set up a web server and a client and pass as ssl:

server = await aiohttp_server(app, ssl=ssl_ctx)
conn = aiohttp.TCPConnector(ssl=client_ssl_ctx)

I'm sorry I don't know how to setup a server and use it.
Can't we leave it like this, it seems to be working fine.

Tests should work offline. Imagine you're traveling w/o any internet connectivity. You should be able to run this on your laptop.

If you could make a suggestion I'd gladly copy paste it.

Actually, you don't have to manually copy suggestions, it's better to click Commit suggestion from the GitHub UI because this way it automatically preserves authorship.

tests/test_resource_warning.py

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

tests/test_resource_warning.py

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

spacemanspiff2007 · 2020-04-27T12:59:08Z

Hm - your internal server changes something so the test does not fail any more.

webknjaz · 2020-04-27T13:09:58Z

Hm - your internal server changes something so the test does not fail any more.

Hm... https://heise.de responds with Connection: keep-alive, I wonder if the test server does that. Could you please inspect this?

spacemanspiff2007 · 2020-04-27T18:06:35Z

Sorry - I don't know much about http connections and don't know how to do that.
I just observed the behavior in the logs of my application.
Maybe you can ping someone who could help?

spacemanspiff2007 · 2020-05-06T08:35:41Z

@webknjaz : Shouldn't we revert the test to the external server solution? At least then it's failing reproducible and can be fixed.

spacemanspiff2007 · 2020-08-23T10:59:24Z

This issue is still relevant - how shall we proceed?

samuelcolvin · 2020-10-16T10:54:49Z

Any update on this, I'm having the same problem.

asvetlov · 2020-10-16T12:00:54Z

The master branch has the reimplementation of client session/connector closing which waits for the actual protocol's connection_lost() and graceful transport's shutdown.
Unfortunately, the work is not 100% finished, the branch is not ready for release yet.

webknjaz · 2020-10-25T18:28:50Z

@asvetlov why did you close this? I think it's a good foundation for the regression test we need.

asvetlov · 2020-10-26T09:40:38Z

As you wish.
Let's keep it open.
I'm not sure that the test should be present as-is. Also, I don't like print() call here

CLAassistant · 2020-11-16T10:16:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ asvetlov
❌ spacemanspiff2007
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Test for ResourceWarning:unclosed transport

81fb892

#4703

spacemanspiff2007 requested a review from asvetlov as a code owner April 26, 2020 09:58

flake fixes and CHANGES

752917d

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 26, 2020

spacemanspiff2007 added 2 commits April 26, 2020 13:15

more flake fixes

9683deb

last flake fix

efcab08

webknjaz reviewed Apr 26, 2020

View reviewed changes

tests/test_resource_warning.py Outdated Show resolved Hide resolved

webknjaz reviewed Apr 26, 2020

View reviewed changes

tests/test_resource_warning.py Outdated Show resolved Hide resolved

spacemanspiff2007 added 3 commits April 26, 2020 15:42

simplified testcase

5048383

removed loop

80609bb

test changes

97cfbda

webknjaz reviewed Apr 26, 2020

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

update testcase

07dbbf6