bpo-47015: Update test_os from asyncore to asyncio by arhadthedev · Pull Request #31876 · python/cpython · GitHub

PEP 594 – Removing dead batteries from the standard library removes asyncore and asynchat in 3.12 with the following note:

The asyncore module is also used in stdlib tests. The tests for ftplib, logging, smptd, smtplib, and ssl are partly based on asyncore. These tests must be updated to use asyncio or threading.

However, the citation misses the tests for os. So this PR ports test_os.TestSendfile as the only class that uses asyncore to create a dummy sendfile server and communicate with it.

To simplify the review I broke all changes into a few commits to get readable diffs:

Preparation:
1. Change inheritance of the test from TestCase to IsolatedAsyncioTestCase
2. Make os.sendfile async-friendly by moving it into a worker thread to not block an event loop of IsolatedAsyncioTestCase. As a result, sendfile_wrapper that called it is also made async
3. Explicitly specify a global event loop policy because an implicit initialization
  by IsolatedAsyncioTestCase is counted as a test failure (this commit is actually the last)
Replace SendfileTestServer with asyncio.start_server. The client side is left mostly untouched because os.sendfile works directly with a low-level socket ignoring asyncio message loop anyway. "Mostly" because connect is delegated to an event loop to not block the server in the same thread
Cleanup:
1. Convert server-side buffer population from a loop into a list comprehension to be more Pythonic
2. Remove redundant waiting of the client for 220 ready. Instead, we use a guarantee that asyncio.loop.sock_connect yields only after establishing a connection
3. Remove SendfileTestServer with related asyncio imports
4. Remove threading leftovers (asyncio.to_thread maintains its own threading machinery)

https://bugs.python.org/issue47015

Note to myself: despite asyncio.start_server promises to be IPv4-first if family is undefined:

cpython/Lib/asyncio/base_events.py

Line 976 in 9523c0d

async def create_connection(

calls

cpython/Lib/asyncio/base_events.py

Lines 1034 to 1036 in 9523c0d

infos = await self._ensure_resolved(

(host, port), family=family,

type=socket.SOCK_STREAM, proto=proto, flags=flags, loop=self)

that calls

cpython/Lib/asyncio/base_events.py

Line 1400 in 9523c0d

info = _ipaddr_info(host, port, family, type, proto, *address[2:])

that has the following lines:

cpython/Lib/asyncio/base_events.py

Lines 129 to 134 in 9523c0d

if family == socket.AF_UNSPEC:

afs = [socket.AF_INET]

if _HAS_IPv6:

afs.append(socket.AF_INET6)

else:

afs = [family]

, it creates an IPv6-only listening socket if possible. As a result, not only server.sockets[0].getsockname() returns an IPv6 address, the whole server refuses to accept IPv4 client connections. It took me two force-pushes to find out, so my apologises about flukes in the notifications.

Now, I'm sorting out what and why leaks all connected sockets (as Address sanitizer reports) failing all non-Windows runners:

/home/runner/work/cpython/cpython/Lib/unittest/async_case.py:134: ResourceWarning: unclosed <socket.socket fd=8, family=2, type=1, proto=6, laddr=('127.0.0.1', 32881), raddr=('127.0.0.1', 57190)>

/home/runner/work/cpython/cpython/Lib/unittest/async_case.py:134: ResourceWarning: unclosed <socket.socket fd=10, family=2, type=1, proto=6, laddr=('127.0.0.1', 36545), raddr=('127.0.0.1', 42800)>

/home/runner/work/cpython/cpython/Lib/unittest/async_case.py:134: ResourceWarning: unclosed <socket.socket fd=12, family=2, type=1, proto=6, laddr=('127.0.0.1', 35363), raddr=('127.0.0.1', 49040)>

/home/runner/work/cpython/cpython/Lib/unittest/async_case.py:134: ResourceWarning: unclosed <socket.socket fd=14, family=2, type=1, proto=6, laddr=('127.0.0.1', 39391), raddr=('127.0.0.1', 41928)>

/home/runner/work/cpython/cpython/Lib/unittest/async_case.py:134: ResourceWarning: unclosed <socket.socket fd=16, family=2, type=1, proto=6, laddr=('127.0.0.1', 42821), raddr=('127.0.0.1', 42452)>

The failure was a combination of client-side blocking wait of a server and a missing asyncio loop policy. Both problems are fixed.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

Without the grouping, reading code as a prose becomes harder: > Start the server. For a client, create a socket, set it nonblocking, > get a server name that is a name of its listening port, connect to the > server. instead of: > Start the server, its name is a name of its listening port. For > a client, create a socket, set it nonblocking, connect to the server.

The line that set a loop policy is not present any more, so this cleanup step doesn’t seem to apply anymore.

If I understand a failed pre-policy run correctly, the policy must either be manually set to something before any case is runned or be returned back to None before the suit exits. I've chosen the first variant, Andrew proposed to switch to the second.

0:04:29 load avg: 3.56 [300/433/1] test_os failed (env changed) -- running: test_asyncio (1 min 10 sec), test_concurrent_futures (1 min 57 sec)

[a bunch of oks and skips follow]

[...]

423 tests OK.

10 slowest tests:
- test_tools: 4 min 44 sec
- test_concurrent_futures: 3 min 30 sec
- test_peg_generator: 2 min 57 sec
- test_multiprocessing_spawn: 2 min 27 sec
- test_gdb: 1 min 51 sec
- test_asyncio: 1 min 50 sec
- test_multiprocessing_forkserver: 1 min 38 sec
- test_multiprocessing_fork: 1 min 22 sec
- test_regrtest: 1 min 12 sec
- test_statistics: 49.1 sec

1 test altered the execution environment:
    test_os

9 tests skipped:
    test_devpoll test_ioctl test_kqueue test_msilib test_startfile
    test_winconsoleio test_winreg test_winsound test_zipfile64

Total duration: 11 min 50 sec
Tests result: ENV CHANGED
make: *** [Makefile:1795: buildbottest] Error 3
Error: Process completed with exit code 2.

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit 005ad9a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

LGTM. Let's wait for the buildbots fleet to finish to make sure that everything works smoothly.
Network code can lead to subtle test problems.

@asvetlov I fixed a forgotten self found by AMD64 FreeBSD.

The second runner, ARM64 macOS, failed on test_asyncio that is unrelated:

======================================================================
ERROR: test_get_cancelled (test.test_asyncio.test_queues.QueueGetTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/pull_request.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/tasks.py", line 490, in wait_for
    return fut.result()
           ^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/pull_request.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/queues.py", line 158, in get
    await getter
    ^^^^^^^^^^^^
asyncio.exceptions.CancelledError

Network code can lead to subtle test problems.

Considering that the failed test_flags was marked as @requires_headers_trailers (not sys.platform.startswith("linux") and not sys.platform.startswith("solaris") and not sys.platform.startswith("sunos")), GitHub runners had no chance.

GitHub Ubuntu runner failed with Error: Cache service responded with 400 during upload chunk; I reopened the PR to retrigger the workflow.

I see. Test failures seem unrelated.
Let's run the buildbot fleet again.

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit 03cae53 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Merged. Thanks!

Convert TestSendfile to IsolatedAsyncioTestCase

b4c3d44

bedevere-bot added the awaiting review label Mar 14, 2022

the-knights-who-say-ni added the CLA signed label Mar 14, 2022

bedevere-bot added the tests Tests in the Lib/test dir label Mar 14, 2022

arhadthedev changed the title Update test_os from asyncore to asyncio bpo-47015: Update test_os from asyncore to asyncio Mar 14, 2022

arhadthedev force-pushed the asyncio-test-os branch 2 times, most recently from ae7f8bb to 815cc75 Compare March 15, 2022 19:44

arhadthedev force-pushed the asyncio-test-os branch 3 times, most recently from f106b3a to 0e19efb Compare March 16, 2022 19:59

arhadthedev added 2 commits March 18, 2022 18:04

Make os.sendfile asynchronous

51c5422

Use an asyncio-connected client

ee9dbe4

arhadthedev force-pushed the asyncio-test-os branch from 0e19efb to 04cd9ee Compare March 18, 2022 15:10

Add and use an asyncio server

a3b9deb

arhadthedev force-pushed the asyncio-test-os branch from 04cd9ee to df82a87 Compare March 18, 2022 16:25

arhadthedev added 5 commits March 18, 2022 19:27

Turn client data reading loop into a comprehension

dcb4251

Remove redundant synchronization

4cc7766

Remove an old, asyncore server

8c90479

Remove threading infrastructure

8fc1e78

Add a NEWS entry

9882daf

arhadthedev force-pushed the asyncio-test-os branch from df82a87 to 9882daf Compare March 18, 2022 16:29

arhadthedev added 2 commits March 18, 2022 20:44

Merge branch 'main' into asyncio-test-os

3d66899

Add an explicit asyncio event loop policy

475b7e5

arhadthedev marked this pull request as ready for review March 18, 2022 18:40

merwok reviewed Mar 18, 2022

View reviewed changes

Comment thread Lib/test/test_os.py Outdated

Address a review of merwok

ccb26dd

asvetlov requested changes Mar 18, 2022

View reviewed changes

Comment thread Lib/test/test_os.py Outdated

Comment thread Lib/test/test_os.py Outdated

Comment thread Lib/test/test_os.py Outdated

bedevere-bot removed the awaiting review label Mar 18, 2022

bedevere-bot added the awaiting changes label Mar 18, 2022

arhadthedev added 2 commits March 19, 2022 07:55

Address a review of asvetlov

34f198c

merwok reviewed Mar 19, 2022

View reviewed changes

asvetlov added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 19, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 19, 2022

Fix a call of async_sendfile in test_flags

03cae53

arhadthedev closed this Mar 19, 2022

arhadthedev reopened this Mar 19, 2022

asvetlov added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 19, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 19, 2022

asvetlov merged commit 3ae975f into python:main Mar 20, 2022

bedevere-bot removed the awaiting changes label Mar 20, 2022

arhadthedev deleted the asyncio-test-os branch March 20, 2022 14:49



		def tearDownModule():
		asyncio.set_event_loop_policy(None)

bpo-47015: Update test_os from asyncore to asyncio by arhadthedev · Pull Request #31876 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-47015: Update test_os from asyncore to asyncio - #31876

bpo-47015: Update test_os from asyncore to asyncio#31876asvetlov merged 15 commits intopython:mainpython/cpython:mainfrom arhadthedev:asyncio-test-osarhadthedev/cpython:asyncio-test-osCopy head branch name to clipboard

Conversation

arhadthedev commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arhadthedev commented Mar 15, 2022

Uh oh!

arhadthedev commented Mar 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 18, 2022

Uh oh!

merwok Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

arhadthedev Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 19, 2022

Uh oh!

asvetlov commented Mar 19, 2022

Uh oh!

arhadthedev commented Mar 19, 2022

Uh oh!

arhadthedev commented Mar 19, 2022

Uh oh!

asvetlov commented Mar 19, 2022

Uh oh!

bedevere-bot commented Mar 19, 2022

Uh oh!

asvetlov commented Mar 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-47015: Update test_os from asyncore to asyncio#31876
asvetlov merged 15 commits into
python:mainfrom
arhadthedev:asyncio-test-os

arhadthedev commented Mar 14, 2022 •
edited
Loading

arhadthedev Mar 19, 2022 •
edited
Loading