-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-36488: os.sendfile() on BSD and macOS doesn't return bytes sent on EINTR #12807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm competent enough to review this, but I had some questions.
@@ -8913,6 +8913,8 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) | |||
#else | |||
ret = sendfile(in, out, offset, len, &sf, &sbytes, flags); | |||
#endif | |||
if (sbytes > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm not sure why you're adding this. Would you care to explain or add a comment?
- It seems
sbytes
can be undefined here (on non-Apple platforms), especially ifsendfile
returns with an error? - On Apple, if you get a non-EAGAIN non-EINTR error, perhaps
sbytes
was kept to its original value (which is presumably non-zero)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you're adding this. Would you care to explain or add a comment?
Sorry for the delayed response. I added a comment which should clarify what's going on. Basically sendfile() on OSX/BSD can fail with EINTR, EAGAIN or EBUSY (usually happens when using non-blocking sockets) but the &len
param can be set to something != 0 in case some data was sent (partial send). In this case this PR exits the while loop, ignores the error condition and return the value of &len
(number of bytes being sent) instead of raising an exception. I did this in accordance with "man sendfile" on MACOS and FreeBSB which describes this possibility (sendfile() failure -> errno set + &len
being set).
Basically the downside here is suppressing the error (which may be undesired in case of CTRL+C?) but the upside is to avoid losing information (which is crucial when keeping track of the file offset when sending a file in non-blocking mode). I'm CC-ing @vstinner since he co-authored PEP-0475 with Antoine.
It seems sbytes can be undefined here (on non-Apple platforms), especially if sendfile returns with an error?
See above + sbytes is always defined on BSD/OSX (although not initialized).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to first handle EINTR, and only later check sbytes. Something like that:
while (1) {
...
Py_END_ALLOW_THREADS
if (ret < 0 && errno == EINTR) {
async_err = PyErr_CheckSignals();
if (async_err) break;
/* sendfile() interrupted by a signal: retry */
continue;
}
if (sbytes > 0) {
break;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's basically what I was unsure of (the handling of PyErr_CheckSignals()
). Unfortunately I don't have an OSX / BSD box to test against at the moment though, so I am afraid this will have to wait (but if anyone wants to give it a try be my guest).
I'm not familiar enough with this stuff to give a review. Sorry. |
@giampaolo, please address @pitrou's questions. Thank you! |
https://bugs.python.org/issue36488