bpo-42237: Fix os.sendfile() on illumos

[jstasiak]

https://bugs.python.org/issue42237

[wiedi]

With this patch on SmartOS:

testCount (test.test_socket.SendfileUsingSendfileTest) ... ok
testCountSmall (test.test_socket.SendfileUsingSendfileTest) ... ok
testCountWithOffset (test.test_socket.SendfileUsingSendfileTest) ... ok
testEmptyFileSend (test.test_socket.SendfileUsingSendfileTest) ... ok
testNonBlocking (test.test_socket.SendfileUsingSendfileTest) ... ok
testNonRegularFile (test.test_socket.SendfileUsingSendfileTest) ... ok
testOffset (test.test_socket.SendfileUsingSendfileTest) ... ok
testRegularFile (test.test_socket.SendfileUsingSendfileTest) ... ok
testWithTimeout (test.test_socket.SendfileUsingSendfileTest) ... ok
testWithTimeoutTriggeredSend (test.test_socket.SendfileUsingSendfileTest) ... ok
test_errors (test.test_socket.SendfileUsingSendfileTest) ... ok

----------------------------------------------------------------------
Ran 11 tests in 0.280s

OK

[kulikjak]

Thanks! I tested this on Oracle Solaris and it still works as expected. Checking inside the do while loop is certainly a right move because before it could incorrectly handle EINTR.

You should probably also write a NEWS entry.

Lastly, I don't know if you have to be that verbose with the comments (especially the in one confirmed case the destination socket had a 5 second timeout set and errno was EAGAIN part - it's expected behavior, not an obscure bug), but hey, those are just details ;).

[jstasiak]

I will fiercely defend the verbosity of that comment. :) The illumos sendfile man page even has this in the EINVAL section:

Fewer bytes were transferred than were requested.

which makes partial write + errno set to EAGAIN combination totally unexpected (and quite obscure) to me. As a code reader I found it invaluable to have such things documented (even repeatedly and redundantly) in source code comments so I'm writing comments I myself would like to see there. The only part I'll admit that's too specific is the value of 5 seconds, I think it's about non-zero timeout, naturally.

I thought fixing a bug is not worth a NEWS entry but I'm happy to write one.

[kulikjak]

Ah, you are right. Partial writes are expected, EAGAIN not so much (this might even be an Illumos bug - not just implementation difference).

I am not sure whether the NEWS entry is necessary, but generally, I was asked to write one even for minor fixes (and this is a pretty important fix).

[jstasiak]

Fair enough! Just added the NEWS entry.

[jstasiak]

Regarding code review from core developers and no access to illumos boxes: I reproduced this on a VM using a Vagrant image recommended by the OpenIndiana documentation. The steps as I remember them:

1. Do the vagrant up && vagrant ssh dance
2. sudo pkg install git gcc-10
3. Clone the repository, ./configure etc as usual

[miss-islington]

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

[bedevere-bot]

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

[miss-islington]

Sorry, @jstasiak and @asvetlov, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fd4ed57674c675e05bd5d577dd5047a333c76c78 3.8

[jstasiak]

Thanks @asvetlov, I'll see about manually backporting this to 3.8.

[asvetlov]

Welcome!
Up to you, I don't insist at all but happy to merge if the backport exists.

[bedevere-bot]

GH-23246 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

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

Hi! The buildbot PPC64LE RHEL8 3.9 has failed when building commit 7ae19ef.

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/250/builds/122) and take a look at the build logs.
4. Check if the failure is related to this commit (7ae19ef) 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/250/builds/122

Failed tests:

• test_asyncio

Failed subtests:

• test_create_ssl_connection - test.test_asyncio.test_events.SelectEventLoopTests

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

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/test/test_asyncio/test_events.py", line 620, in test_create_ssl_connection
    self._test_create_ssl_connection(httpd, create_connection,
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/test/test_asyncio/test_events.py", line 579, in _test_create_ssl_connection
    self._basetest_create_ssl_connection(conn_fut, check_sockname,
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/test/test_asyncio/test_events.py", line 571, in _basetest_create_ssl_connection
    self.check_ssl_extra_info(tr, check_sockname, peername)
  File "/home/buildbot/buildarea/3.9.cstratak-RHEL8-ppc64le/build/Lib/test/test_asyncio/test_events.py", line 535, in check_ssl_extra_info
    self.assertIsNotNone(client.get_extra_info('sockname'))
AssertionError: unexpectedly None

[asvetlov]

again sslproto related failure :(
Unrelated to the PR

[jstasiak]

Roger. #23246 is a 3.8 backport of this PR.