Add a new socket.sendfile() method

@gvanrossum

BPO	17552
Nosy	@gvanrossum, @pitrou, @giampaolo, @tiran, @asvetlov, @4kir4, @1st1, @MojoVampire
Files	socket-sendfile1.patch: initial draft socket-sendfile2.patch: update file offset add 'offset' parameter socket-sendfile3.patch: docs, selectors module, always seek() file, use memoryview(), ssl tests bench.py socket-sendfile4.patch: return no. of bytes sent instead of (ok, exc); "sleep" on BlockingIOError socket-sendfile5.patch: fix some issues on windows socket-sendfile6.patch: get rid of "blocksize" and "use_fallback" args, add "count" arg, use fstat() socket-sendfile7.patch: adds send_blocksize argument

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/giampaolo'
closed_at = <Date 2014-06-11.02:04:53.134>
created_at = <Date 2013-03-26.19:27:53.402>
labels = ['type-feature', 'library']
title = 'Add a new socket.sendfile() method'
updated_at = <Date 2014-06-11.02:04:53.054>
user = 'https://github.com/giampaolo'

bugs.python.org fields:

activity = <Date 2014-06-11.02:04:53.054>
actor = 'giampaolo.rodola'
assignee = 'giampaolo.rodola'
closed = True
closed_date = <Date 2014-06-11.02:04:53.134>
closer = 'giampaolo.rodola'
components = ['Library (Lib)']
creation = <Date 2013-03-26.19:27:53.402>
creator = 'giampaolo.rodola'
dependencies = []
files = ['29583', '29734', '34986', '34994', '34997', '35004', '35039', '35507']
hgrepos = []
issue_num = 17552
keywords = ['3.2regression']
message_count = 35.0
messages = ['185292', '185294', '185295', '185303', '186305', '216915', '216968', '216973', '216975', '216979', '217015', '217058', '217080', '217081', '217082', '217090', '217099', '217104', '217120', '217121', '217128', '217143', '217144', '217156', '217165', '219926', '219927', '219928', '219929', '220173', '220191', '220195', '220202', '220220', '220226']
nosy_count = 12.0
nosy_names = ['gvanrossum', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'josiah.carlson', 'asvetlov', 'neologix', 'akira', 'rosslagerwall', 'python-dev', 'yselivanov', 'josh.r']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue17552'
versions = ['Python 3.5']

This is based on progress made in bpo-13564 and aims to provide a new sendfile() method for the socket class taking advantage of high-performance "zero-copy" os.sendfile() available on most POSIX platforms.
A wrapper on top of os.sendfile() appears to be convenient because getting everything right is a bit tricky. Also we can avoid code duplication in case we want to add sendfile() support to ftplib (bpo-13564) and httplib.

=== THE API ===

Attached is a draft patch proposing an API which:

uses os.sendfile() whenever available and usable (a mmap-like file is required)
if not falls back on using plain socket.sendall()
returns a tuple of two elements which indicates whether sendfile() was used and an exception instance in case it wasn't on account of an internal error, if any
does not support non-blocking sockets (will raise ValueError)

=== ALTERNATE API ===

In case the transfer is interrupted halfway because of an error the user has no clue on how many bytes were sent (and file.tell() won't tell =)). As such it probably makes sense to provide a custom socket.SendfileError exception encapsulating the original exception and the bytes sent as arguments.
For the same reason the returned tuple should probably include the number of bytes transmitted.

=== WINDOWS SUPPORT ===

Further development may include Windows support by using TransmitFile (http://msdn.microsoft.com/en-us/library/windows/desktop/ms740565(v=vs.85).aspx). Once we agree on an API I might start looking into Windows code (which appears to be quite tricky btw).

Any feedback is very welcome.

A couple of comments:

I don't understand the point of the second member in the tuple
the timeout logic should be fixed so that the total operation time doesn't exceed the timeout, rather than each iteration (in other words, a deadline should be computed at the start and then the poll / select calls be issued in accordance with that deadline)
non-blocking sockets could be supported by returning partial writes, and letting the caller pass an offset argument
as a vocabulary nit, I don't really understand what "mmap-like file" means

I don't understand the "running out of FDs" thing. select() is limited to FDs less than FD_SETSIZE, but I don't see how you could get EMFILE (select() will return a ValueError)
is there any platform with sendfile() which doesn't support poll()?
I still don't see the point of calling sendfile() with a block size. Everybody just uses st_size, and that's how it's supposed to be done.

I don't understand the point of the second member in the tuple

The 'exception' member can be useful to know the reason why sendfile() failed and send() was used as fallback.

the timeout logic should be fixed so that the total operation
time doesn't exceed the timeout, rather than each iteration

Do you mean that if the user sets a timeout=2 and the whole transfer takes longer than that you expect a timeout exception?
That's pretty unusual (e.g. ftplib does not behave like that).

non-blocking sockets could be supported by returning partial writes,
and letting the caller pass an offset argument

I see little value in supporting non-blocking sockets because someone willing to do that usually wants to use sendfile() directly (no wrappers involved) as they are forced to handle errors and take count of transmitted bytes in their own code anyway.
Here's an example of how sendfile is supposed to be used with non-blocking sockets:
https://code.google.com/p/pyftpdlib/source/browse/tags/release-0.7.0/pyftpdlib/ftpserver.py#1035
An extra layer such as the one proposed in my patch is completely useless and also slow considering the amount of instructions before the actual sendfile() call.
It is however a good idea to provide an 'offset' argument.

as a vocabulary nit, I don't really understand what "mmap-like file" means

Will turn that into "regular files".

I don't see how you could get EMFILE (select() will return a ValueError)

Right. Will fix that.

is there any platform with sendfile() which doesn't support poll()?

No idea. I made a quick tour on snakebite and apparently all of the provided platforms have poll().

I still don't see the point of calling sendfile() with a block size

I will make some actual benchmarks and fix that later if makes sense.
Assuming providing a high blocksize (say 1MB) makes no difference in terms of CPU usage it's better to keep that for consistency with sendall() in case we fallback on using it.

New patch in attachment includes a new 'offset' parameter, new tests and also update file offset on return or in case of error so that file.tell() can be used to tell how many bytes were transmitted at any time.
This way we'll avoid using a custom exception.

In summary, the API looks like this.

Transfer ok:

>>> file = open('somefile', 'rb')
>>> s = socket.socket()
>>> sock.sendfile(file)
(True, None)
>>> file.tell()
20000000
>>>

...and in case sendfile() could not be used internally because file was not a regular file:

>>> file = io.BytesIO(b'x' * 1*1024*1024)
>>> sock.sendfile(file)
(False, UnsupportedOperation('fileno',))
>>> file.tell()
20000000
>>>

I still haven't looked into TransmitFile on Windows as I have to figure out how to compile Python 3.4 on Windows.

New patch in attachment. Changes:

docs
replaced select() / poll() with the new selectors module
file position is always updated both on return and on error; this means file.tell() is the designated way to know how many bytes were sent
replaced sendall() with send() so that we can count the number of bytes transmitted (related and rejected proposal: https://mail.python.org/pipermail/python-ideas/2014-April/027689.html)
send() now uses memoryview() for better performances to re-transmit data which was not sent by the first send() call
pre-emptively raise exception if file is not opened in binary mode
tests for ssl module

I've tried to work on Windows TransmitFile support but I got stuck as I'm not sure how to convert a file object into a HANDLE in C. I suppose Windows support can also be added later as a separate ticket and in the meantime I'd like to push this forward.

Open questions:

Is the current return value desirable (do we really care if os.sendfile() was used internally?)? Should the returned tuple also include the number transmitted bytes?
default blocksize: Charles-François was suggesting to remove the blocksize argument; FWIW I've made some quick benchmarks by using "time" cmdline utility with different blocksizes and I didn't notice substantial difference. I still think a blocksize parameter is necessary in case we fallback on using send() and also for consistency with ftplib's storbinary() method which will be involved later (bpo-13564).

Attached is a simple benchmark script transmitting a 100MB file.
On my Linux box sendfile() is almost twice as fast as send():

send()
real     0.0613s
user     0.0100s
sys      0.0900s
total    0.1000s

sendfile()
real     0.0318s
user     0.0000s
sys      0.0500s
total    0.0500s

For TransmitFile support, the Windows function to turn an integer file descriptor into a WinAPI file HANDLE should be _get_osfhandle: http://msdn.microsoft.com/en-us/library/ks2530z6.aspx

Should socket.sendfile() always return number of bytes sent because file.tell() may be changed by something else that uses the same file descriptor?

What happens if the file grows?

Instead of returning (was_os_sendfile_used, os_sendfile_error), you could specify no_fallback=False that could be set to True to assert that the fallback is not used (with no_fallback=True the os_sendfile_error is raised instead of using socket.send() as a fallback).

If possible; always include number of bytes sent in any error that is raised.

Instead of returning [...] you could specify no_fallback=False that
could be set to True to assert that the fallback is not used
[...] and return the number of bytes sent.

Good idea, thanks, that is much better indeed. Updated patch is in attachment.

[...] file.tell() may be changed by something else that uses
the same file descriptor? What happens if the file grows?

I would say that is a use case we should explicitly not support as it probably implies you're doing something you're not supposed to.

If possible always include the number of bytes sent in any error that is raised.

That's similar to my recent (rejected) proposal for socket.sendall():
https://mail.python.org/pipermail/python-ideas/2014-April/027689.html
IMO the patch as it stands is fine as you can determine the number of bytes which were sent either by using the function return value or file.tell() (in case of error).
Also, updating the file offset on exit makes the sendfile() implementation behave exactly like send().

Yet another patch fixing some problems on Windows.
Hopefully this should be the last one as for what concerns the POSIX systems. As such I would kindly ask for a review and/or further suggestions.

Given the opinions expressed so far I:

got rid of the "blocksize" parameter
got rid of the "use_fallback" parameter
added a "count" parameter
used os.fstat() to figure out the total file size and passed it directly to sendfile()

I'm attaching socket-sendfile6.patch which includes docs and many new tests.
Hopefully this should be the final one (yet to review though).

Looking back at this I think a "send_blocksize" argument is necessary after all. shutil.copyfileobj() has it, so is ftplib.FTP.storbinary() and httplib (bpo-13559) which will both be using socket.sendfile() once it gets included.
Updated patch is in attachment. If there are no other objections I'd be for committing this next week or something as I'm pretty confident with the current implementation.

Looking back at this I think a "send_blocksize" argument is necessary after all. shutil.copyfileobj() has it, so is ftplib.FTP.storbinary() and httplib (bpo-13559) which will both be using socket.sendfile() once it gets included.

Those APIs are really poor, please don't cripple sendfile() to mirror them.

Once again, a send_blocksize argument doesn't make sense, you won't
find it anywhere else. All that's needed is start offset and a
size/length argument.

I agree it is not necessary for sendfile() (you were right).
Do not introducing it for send(), though, poses some questions.
For instance, should we deprecate or ignore 'blocksize' argument in ftplib as well?
Generally speaking, when using send() there are circumstances where you might want to adjust the number of bytes to read from the file, for instance:

1: set a small blocksize (say 512 bytes) on systems where you have a limited amount of memory
2: set a big blocksize (say 256000) in order to speed up the transfer / use less CPU cycles; on very fast networks (e.g. LANs) this may result in a considerable speedup (I know 'cause I conducted these kind of tests in pyftpdlib: https://code.google.com/p/pyftpdlib/issues/detail?id=94).

...speaking of which, now that I look back at those benchmarks it looks like 65536 bytes is the best compromise (in my latest patch I used 16348).

Charles, Antoine, any final thought about this given the reasons I stated above? If you're still -1 about adding 'send_blocksize' argument I guess I can get rid of it and perhaps reintroduce it later if we see there's demand for it.

I agree it is not necessary for sendfile() (you were right).

Good we agree :-)

Do not introducing it for send(), though, poses some questions.
For instance, should we deprecate or ignore 'blocksize' argument in ftplib as well?

Honestly, we should deprecate the whole ftplib module :-)
More seriously, it's really low-level, I don't understand the point of
this whole callback-based API:
FTP.storbinary(command, file[, blocksize, callback, rest])Why not simply a:
FTP.store(source, target, binary=True)

If you have time and it interest you, trying to improve this module
would be great :-)

Generally speaking, when using send() there are circumstances where you might want to adjust the number of bytes to read from the file, for instance:

1: set a small blocksize (say 512 bytes) on systems where you have a limited amount of memory

2: set a big blocksize (say 256000) in order to speed up the transfer / use less CPU cycles; on very fast networks (e.g. LANs) this may result in a considerable speedup (I know 'cause I conducted these kind of tests in pyftpdlib: https://code.google.com/p/pyftpdlib/issues/detail?id=94).

I agree, but both points are addressed by sendfile(): internally, the
kernel will use whatever block size it likes, depending on the source
file type, page size, target NIC ring buffer size.

So to reply to your above question, I wouldn't feel too bad about
simply ignoring the blocksize argument in e.g. shutil.copyfile(). For
ftplib, it's a little harder since we're supposed to support an
optional callback, but calling a callback for every block will drive
performance down...

So I'd really like it if you could push the version without the
blocksize, and then we'll see (and hopefully fix the non-sensical
libraries that depend on it).

I agree, but both points are addressed by sendfile()

I'm talking about send(), not sendfile().
Please remember that send() will be used as the default on Windows or when non-regular files are passed to the function. My argument is about introducing an argument to use specifically with send(), not sendfile().
In summary:

sendfile(self, file, offset=0, count=None, send_blocksize=16384):
"""
[...]
If os.sendfile() is not available (e.g. Windows) or file is not
a regular file socket.send() will be used instead.
[...]
*send_blocksize* is the maximum number of bytes to transmit at
one time in case socket.send() is used.
"""

Honestly, we should deprecate the whole ftplib module :-)
More seriously, it's really low-level, I don't understand the point of
this whole callback-based API:
FTP.storbinary(command, file[, blocksize, callback, rest])
Why not simply a:
FTP.store(source, target, binary=True)

ftplib module API may be a bit rusty but there's a reason why it was designed like that.
'callback' and 'blocksize' arguments can be used to implement progress bars, in-place transformations of the source file's data and bandwidth throttling (by having your callback 'sleep' if more than N bytes were sent in the last second). 'rest' argument is necessary for resuming uploads.
I'm not saying ftplib API cannot be improved: maybe we can provide two higher level "put" and "get" methods but please let's discuss that into another thread.

> I agree, but both points are addressed by sendfile()

I'm talking about send(), not sendfile().
Please remember that send() will be used as the default on Windows or when non-regular files are passed to the function. My argument is about introducing an argument to use specifically with send(), not sendfile().

Which makes even less sense if it's not needed for sendfile() :-)
I really don't see why we're worrying so much about allocating 8K or
16K buffers, it's really ridiculous. For example, buffered file I/O
uses a default block size of 8K. We're not targeting embedded systems.

OK then, I'll trust your judgement. I'll use 8K as the default and will commit the patch soon.

New changeset 001895c39fea by Giampaolo Rodola' in branch 'default':
fix issue bpo-17552: add socket.sendfile() method allowing to send a file over a socket by using high-performance os.sendfile() on UNIX. Patch by Giampaolo Rodola'·
http://hg.python.org/cpython/rev/001895c39fea

Navigation Menu

Repository navigation

Add a new socket.sendfile() method #61752

13 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Add a new socket.sendfile() method #61752

Description

Activity

giampaolo commented on Mar 26, 2013

pitrou commented on Mar 26, 2013

neologix commented on Mar 26, 2013

giampaolo commented on Mar 26, 2013

giampaolo commented on Apr 8, 2013

giampaolo commented on Apr 20, 2014

giampaolo commented on Apr 21, 2014

MojoVampire commented on Apr 21, 2014

4kir4 commented on Apr 21, 2014

giampaolo commented on Apr 22, 2014

giampaolo commented on Apr 22, 2014

13 remaining items

giampaolo commented on Apr 25, 2014

giampaolo commented on Jun 7, 2014

neologix commented on Jun 7, 2014

giampaolo commented on Jun 7, 2014

giampaolo commented on Jun 7, 2014

giampaolo commented on Jun 10, 2014

neologix commented on Jun 10, 2014

giampaolo commented on Jun 10, 2014

neologix commented on Jun 10, 2014

giampaolo commented on Jun 11, 2014

python-dev commented on Jun 11, 2014

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions