Skip to content

bpo-29659: Expose copyfileobj() length arg for public use #328

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.

Sign up for GitHub

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
goodboy wants to merge 3 commits into from
Closed

bpo-29659: Expose copyfileobj() length arg for public use #328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fb7091
Expose copyfileobj() `length` arg for public use
Feb 27, 2017
24b2f57
Document exposure of the `copyfileobj` `length` arg in shutil
Mar 3, 2017
8daf7a8
Add test to using new `length` kwarg
Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Document exposure of the copyfileobj length arg in shutil
  • Loading branch information
Tyler Goodlet
Tyler Goodlet committed Nov 20, 2017
commit 24b2f57c9bc52d1d4c96949ad9f2b74accb1aec5
40 changes: 30 additions & 10 deletions Doc/library/shutil.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ Directory and files operations

.. function:: copyfileobj(fsrc, fdst[, length])

Copy the contents of the file-like object *fsrc* to the file-like object *fdst*.
The integer *length*, if given, is the buffer size. In particular, a negative
*length* value means to copy the data without looping over the source data in
chunks; by default the data is read in chunks to avoid uncontrolled memory
consumption. Note that if the current file position of the *fsrc* object is not
0, only the contents from the current file position to the end of the file will
be copied.
Copy the contents of the file-like object *fsrc* to the file-like object
*fdst*. Only the contents from the current file position to the end of
the file will be copied.

The integer *length*, if given, is the buffer size; the default value
in bytes is 16 KiB. A negative *length* value means to copy the data without
looping over the source data in chunks; by default the data is read in
chunks to avoid uncontrolled memory consumption.

@vstinner vstinner Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"A negative length value means to copy the data without looping over the source data in chunks"

I dislike this definition. In practice, negative means "unlimited" buffer size: the whole input file is loaded into memory.

I'm not sure that it's a good practice to try to load files of unknown size into memory.

I suggest to remove this feature which seems more like a side effect than a carefully designed API.

If you want to get fast copy, pass a very large length like 1 GB. But if Python starts to load 1 TB into memory, it's likely to crash the system... At least, to slow down the system, a lot.


.. function:: copyfile(src, dst, *, follow_symlinks=True)
.. function:: copyfile(src, dst, *, follow_symlinks=True, length=None)

Copy the contents (no metadata) of the file named *src* to a file named
*dst* and return *dst*. *src* and *dst* are path names given as strings.
Expand All @@ -65,6 +65,9 @@ Directory and files operations
a new symbolic link will be created instead of copying the
file *src* points to.

The integer *length*, if given, is the in-memory buffer size; the default
value in bytes is 16 KiB (see :func:`shutil.copyfileobj`).

.. versionchanged:: 3.3
:exc:`IOError` used to be raised instead of :exc:`OSError`.
Added *follow_symlinks* argument.
Expand All @@ -74,6 +77,9 @@ Directory and files operations
Raise :exc:`SameFileError` instead of :exc:`Error`. Since the former is
a subclass of the latter, this change is backward compatible.

.. versionchanged:: 3.7
Added *length* parameter.


.. exception:: SameFileError

Expand Down Expand Up @@ -141,7 +147,7 @@ Directory and files operations
.. versionchanged:: 3.3
Added *follow_symlinks* argument and support for Linux extended attributes.

.. function:: copy(src, dst, *, follow_symlinks=True)
.. function:: copy(src, dst, *, follow_symlinks=True, length=None)

Copies the file *src* to the file or directory *dst*. *src* and *dst*
should be strings. If *dst* specifies a directory, the file will be
Expand All @@ -153,6 +159,9 @@ Directory and files operations
is true and *src* is a symbolic link, *dst* will be a copy of
the file *src* refers to.

The integer *length*, if given, is the in-memory buffer size; the default
value in bytes is 16 KiB (see :func:`shutil.copyfileobj`).

:func:`~shutil.copy` copies the file data and the file's permission
mode (see :func:`os.chmod`). Other metadata, like the
file's creation and modification times, is not preserved.
Expand All @@ -163,7 +172,11 @@ Directory and files operations
Added *follow_symlinks* argument.
Now returns path to the newly created file.

.. function:: copy2(src, dst, *, follow_symlinks=True)
.. versionchanged:: 3.7
Added `length` parameter


.. function:: copy2(src, dst, *, follow_symlinks=True, length=None)

Identical to :func:`~shutil.copy` except that :func:`copy2`
also attempts to preserve all file metadata.
Expand All @@ -176,6 +189,9 @@ Directory and files operations
unavailable, :func:`copy2` will preserve all the metadata
it can; :func:`copy2` never returns failure.

The integer *length*, if given, is the in-memory buffer size; the default
value in bytes is 16 KiB (see :func:`shutil.copyfileobj`).

:func:`copy2` uses :func:`copystat` to copy the file metadata.
Please see :func:`copystat` for more information
about platform support for modifying symbolic link metadata.
Expand All @@ -185,6 +201,10 @@ Directory and files operations
file system attributes too (currently Linux only).
Now returns path to the newly created file.

.. versionchanged:: 3.7
Added `length` parameter


.. function:: ignore_patterns(\*patterns)

This factory function creates a function that can be used as a callable for
Expand Down
14 changes: 10 additions & 4 deletions Lib/shutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ class RegistryError(Exception):
def copyfileobj(fsrc, fdst, length=None):

@AraHaan AraHaan Nov 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

def copyfileobj(fsrc, fdst, length=16*1024):

I personally prefer this way in my code to concentrate 2 lines worth of code into one.

    if not length:
        length = 16 * 1024

It looks a little nicer and not to mention saves lines of code that basically do the same thing.
And yes I am also guilty of doing it this way and then people started hating me for it and calling my a bad programmer for it.

"""Copy data from file-like object `fsrc` to file-like object `fdst`.

An in-memory buffer size can be set with `length`; the default is 16 KB.
An in-memory buffer size in bytes can be set with `length`; the default is
16 KiB.
"""
if not length:
length = 16*1024
length = 16 * 1024
while 1:

@vstinner vstinner Dec 13, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure that it's ok to use a loop if the length is negative. I suggest to have a special case for negative value calling read() (no parameter) only once.

buf = fsrc.read(length)
if not buf:
Expand All @@ -104,8 +105,8 @@ def copyfile(src, dst, *, follow_symlinks=True, length=None):
If follow_symlinks is not set and src is a symbolic link, a new
symlink will be created instead of copying the file it points to.

An in memory buffer size can be set with `length`; the default is 16 kB.

An in-memory buffer size in bytes can be set with `length`; the default is
16 KiB.
"""
if _samefile(src, dst):
raise SameFileError("{!r} and {!r} are the same file".format(src, dst))
Expand Down Expand Up @@ -242,6 +243,9 @@ def copy(src, dst, *, follow_symlinks=True, length=None):
If source and destination are the same file, a SameFileError will be
raised.

An in-memory buffer size in bytes can be set with `length`; the default is
16 KiB.

"""
if os.path.isdir(dst):
dst = os.path.join(dst, os.path.basename(src))
Expand All @@ -258,6 +262,8 @@ def copy2(src, dst, *, follow_symlinks=True, length=None):
If follow_symlinks is false, symlinks won't be followed. This
resembles GNU's "cp -P src dst".

An in-memory buffer size in bytes can be set with `length`; the default is
16 KiB.
"""
if os.path.isdir(dst):
dst = os.path.join(dst, os.path.basename(src))
Expand Down