bpo-42237: Fix os.sendfile() on illumos#23154
Conversation
wiedi
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
|
Thanks! I tested this on Oracle Solaris and it still works as expected. Checking inside the 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 ;). |
Sorry, something went wrong.
|
I will fiercely defend the verbosity of that comment. :) The illumos sendfile man page even has this in the which makes partial write + errno set to I thought fixing a bug is not worth a NEWS entry but I'm happy to write one. |
Sorry, something went wrong.
|
Ah, you are right. Partial writes are expected, 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). |
Sorry, something went wrong.
|
Fair enough! Just added the NEWS entry. |
Sorry, something went wrong.
|
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:
|
Sorry, something went wrong.
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <jakub@stasiak.at>
|
Sorry, @jstasiak and @asvetlov, I could not cleanly backport this to |
Sorry, something went wrong.
|
Thanks @asvetlov, I'll see about manually backporting this to 3.8. |
Sorry, something went wrong.
|
Welcome! |
Sorry, something went wrong.
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <jakub@stasiak.at>
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot PPC64LE RHEL8 3.9 has failed when building commit 7ae19ef. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/250/builds/122 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (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
|
Sorry, something went wrong.
|
again sslproto related failure :( |
Sorry, something went wrong.
https://bugs.python.org/issue42237