bpo-41682: fixed flaky test test_sendfile_close_peer_in_the_middle_of_receiving

[kumaraditya303]

https://bugs.python.org/issue41682

[kumaraditya303]

...And we finally have green CI 🎉

[kumaraditya303]

cc @gvanrossum @vstinner @pablogsal

[erlend-aasland]

Assuming Guido's reasoning is correct, this is IMO the correct fix. LGTM!

(But note that this test has been flakey for two years; maybe there was another reason for the failures before?)

[erlend-aasland]

...And we finally have green CI 🎉

Actually, it has been green since Nikita's #30801 😉 Please sync with main and remove the Windows guard added by #30801. If this PR fixes the test on Windows, we no longer need the guard.

[kumaraditya303]

Assuming Guido's reasoning is correct, this is IMO the correct fix. LGTM!

(But note that this test has been flakey for two years; maybe there was another reason for the failures before?)

I was already trying to find the size of DATA based on some stack overflow answers before Guido provided his reasoning as I once tried making DATA very high and it worked so I knew it was about fine tuning the DATA length.

[kumaraditya303]

...And we finally have green CI 🎉

Actually, it has been green since Nikita's #30801 😉 Please sync with main and remove the Windows guard added by #30801. If this PR fixes the test on Windows, we no longer need the guard.

@erlend-aasland
I already removed the skip for windows, no?

[erlend-aasland]

I already removed the skip for windows, no?

Great, I see it. Thanks.

[Fidget-Spinner]

I'm gonna test with buildbots. For now, please hold off on committing anything (well you can, but it'll make the buildbots status harder to track ;).

If this works, we need to give you an award for saving thousands of contributors from this pain.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 369390f 🤖

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

[erlend-aasland]

If this works, we need to give you an award for saving thousands of contributors from this pain.

We could consider giving this patch a NEWS entry, given the amount of frustration and gray hair this test has caused 😆

[gvanrossum]

I'll leave this in Erlend's capable hands.

[erlend-aasland]

Thanks for you input, @asvetlov!

I would make the PR title and message more accurate before merging; maintaining a clear and self-documenting git history really helps when issues pop up again (I suspect this might do in a Windows release or five). Here's a quick 'n dirty suggestion, but feel free to word it differently:

bpo-NNNNN: Double the size of test_asyncio.test_sendfile data buffer

Some Windows versions use a large internal transmit buffer, which can
make tests that rely on fragmented transmits flakey (for example
test_sendfile_close_peer_in_the_middle_of_receiving).

We now use a buffer slightly larger than 256 KiB.

Other than that, I have no further remarks. (The failed buildbot is not related to this PR.)

Here's your CI Janitor Award: 🏆🥇 Thanks for keeping the bots green! Thanks, Guido and Kumar, for the reasoning and the patch.

(Andrew, Guido, or Ken can merge; I don't have the commit bit.)

[erlend-aasland]

(The bot that edit our PR comments in order to create bpo links can be very, very irritating at times...)

[miss-islington]

Thanks @kumaraditya303 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

[pablogsal]

Merged ;)

[bedevere-bot]

GH-30860 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-30861 is a backport of this pull request to the 3.9 branch.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 macOS 3.x has failed when building commit 1c705fd.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/725/builds/837) and take a look at the build logs.
4. Check if the failure is related to this commit (1c705fd) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/725/builds/837

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

411 tests OK.

10 slowest tests:

• test_concurrent_futures: 3 min 49 sec
• test_multiprocessing_spawn: 2 min 33 sec
• test_multiprocessing_forkserver: 1 min 51 sec
• test_unparse: 1 min 24 sec
• test_asyncio: 1 min 18 sec
• test_tokenize: 1 min 13 sec
• test_logging: 1 min
• test_lib2to3: 54.5 sec
• test_capi: 52.4 sec
• test_ssl: 47.5 sec

1 test altered the execution environment:
test_ftplib

17 tests skipped:
test_dbm_gnu test_devpoll test_epoll test_gdb test_ioctl
test_msilib test_multiprocessing_fork test_ossaudiodev test_spwd
test_startfile test_tix test_tk test_ttk_guionly test_winconsoleio
test_winreg test_winsound test_zipfile64

Total duration: 9 min 17 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncore.py", line 90, in read
    obj.handle_read_event()
    ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ftplib.py", line 384, in handle_read_event
    self._do_ssl_handshake()
    ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ftplib.py", line 345, in _do_ssl_handshake
    self.socket.do_handshake()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/ssl.py", line 1345, in do_handshake
    self._sslobj.do_handshake()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
ssl.SSLZeroReturnError: TLS/SSL connection has been closed (EOF) (_ssl.c:998)


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_9d7d4089'


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_774f2276'


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/threading.py", line 1031, in _bootstrap_inner
    self.run()
    ^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ftplib.py", line 298, in run
    asyncore.loop(timeout=0.1, count=1)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncore.py", line 214, in loop
    poll_fun(timeout, map)
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncore.py", line 157, in poll
    read(obj)
    ^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncore.py", line 94, in read
    obj.handle_error()
    ^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ftplib.py", line 421, in handle_error
    raise Exception
    ^^^^^^^^^^^^^^^
Exception
k

[vstinner]

@pablogsal: Merged ;)

Be careful next time you merge a PR, the merged commit title is just "fixed flaky test" which doesn't mention test_asyncio or the bpo number, so no comment was added to https://bugs.python.org/issue41682

That's the annoying part with GH PR: it's possible to change the title and description in GitHub, and then get I completly different title and description when you merge it...

[pablogsal]

@pablogsal: Merged ;)

Be careful next time you merge a PR, the merged commit title is just "fixed flaky test" which doesn't mention test_asyncio or the bpo number, so no comment was added to https://bugs.python.org/issue41682

That's the annoying part with GH PR: it's possible to change the title and description in GitHub, and then get I completly different title and description when you merge it...

I also used the GitHub phone app to merge it which o think made things even more confusing as I didn't see that the commit title would be different :(

[vstinner]

I agree, the UI on a phone is "not great" :-( Especially for a merge operation.