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
|
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 ;). |
|
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. |
|
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). |
|
Fair enough! Just added the NEWS entry. |
|
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:
|
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <[email protected]>
|
GH-23244 is a backport of this pull request to the 3.9 branch. |
|
Sorry, @jstasiak and @asvetlov, I could not cleanly backport this to |
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <[email protected]>
|
Thanks @asvetlov, I'll see about manually backporting this to 3.8. |
|
Welcome! |
|
GH-23246 is a backport of this pull request to the 3.8 branch. |
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <[email protected]>
|
|
again sslproto related failure :( |
|
Roger. #23246 is a 3.8 backport of this PR. |
(cherry picked from commit fd4ed57) Co-authored-by: Jakub Stasiak <[email protected]>
https://bugs.python.org/issue42237