bpo-36488: os.sendfile() on BSD and macOS doesn't return bytes sent on EINTR

[giampaolo]

https://bugs.python.org/issue36488

[pitrou]

I'm not sure I'm competent enough to review this, but I had some questions.

[pitrou]

1. I'm not sure why you're adding this. Would you care to explain or add a comment?
2. It seems sbytes can be undefined here (on non-Apple platforms), especially if sendfile returns with an error?
3. On Apple, if you get a non-EAGAIN non-EINTR error, perhaps sbytes was kept to its original value (which is presumably non-zero)?

[giampaolo]

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).

[vstinner]

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; 
  }
}

[giampaolo]

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).

[ericsnowcurrently]

I'm not familiar enough with this stuff to give a review. Sorry.

[csabella]

@giampaolo, please address @pitrou's questions. Thank you!