Skip to content

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

giampaolo
Copy link
Contributor

@giampaolo giampaolo commented Apr 12, 2019

@brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Apr 17, 2019
Copy link
Member

@pitrou pitrou left a 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@giampaolo giampaolo Jan 16, 2020

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

Copy link
Member

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

Copy link
Contributor Author

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

@ericsnowcurrently
Copy link
Member

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

@ericsnowcurrently ericsnowcurrently removed their request for review June 14, 2019 16:27
@csabella
Copy link
Contributor

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

@giampaolo giampaolo closed this by deleting the head repository Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants