Skip to content
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

Fully implement IOBase abstract on SpooledTemporaryFile #70363

Closed
GFernie mannequin opened this issue Jan 21, 2016 · 14 comments
Closed

Fully implement IOBase abstract on SpooledTemporaryFile #70363

GFernie mannequin opened this issue Jan 21, 2016 · 14 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir

Comments

@GFernie
Copy link
Mannequin

GFernie mannequin commented Jan 21, 2016

BPO 26175
Nosy @terryjreedy, @giampaolo, @merwok, @pR0Ps, @vadmium, @ztane, @GFernie, @iritkatriel, @danieldjewell, @adriangb
PRs
  • bpo-26175: Fix SpooledTemporaryFile IOBase abstract #3249
  • bpo-26175: Implement io.IOBase interface for SpooledTemporaryFile #29560
  • Files
  • fix-SpooledTemporaryFile-IOBase-abstract.patch: Patch
  • 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 = None
    closed_at = None
    created_at = <Date 2016-01-21.21:54:34.199>
    labels = ['type-bug', 'library', '3.11']
    title = 'Fully implement IOBase abstract on SpooledTemporaryFile'
    updated_at = <Date 2022-02-28.19:38:17.577>
    user = 'https://github.com/GFernie'

    bugs.python.org fields:

    activity = <Date 2022-02-28.19:38:17.577>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2016-01-21.21:54:34.199>
    creator = 'Gary Fernie'
    dependencies = []
    files = ['41685']
    hgrepos = []
    issue_num = 26175
    keywords = ['patch']
    message_count = 14.0
    messages = ['258769', '258782', '310298', '319089', '319145', '328908', '328911', '368801', '379957', '379958', '413973', '414033', '414035', '414218']
    nosy_count = 11.0
    nosy_names = ['terry.reedy', 'giampaolo.rodola', 'eric.araujo', 'pR0Ps', 'martin.panter', 'ztane', 'Gary Fernie', 'nubirstein', 'iritkatriel', 'danieljewell', 'adriangb']
    pr_nums = ['3249', '29560']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26175'
    versions = ['Python 3.11']

    @GFernie
    Copy link
    Mannequin Author

    GFernie mannequin commented Jan 21, 2016

    SpooledTemporaryFile does not fully satisfy the abstract for IOBase.
    Namely, seekable, readable, and writable are missing.

    This was discovered when seeking a SpooledTemporaryFile-backed lzma file.
    You may quickly repro this:
    lzma.open(SpooledTemporaryFile()).seek(0)

    @GFernie GFernie mannequin added the type-bug An unexpected behavior, bug, or error label Jan 21, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Jan 22, 2016

    It would be nice to add test cases for this.

    Looking at <https://docs.python.org/release/3.4.3/library/tempfile.html#tempfile.SpooledTemporaryFile\>, there is a version changed notice for the truncate() method. So perhaps this should be added as a new feature for 3.6. On the other hand, the documentation doesn’t mention the specific API, so I would assume the relevant IOBase subclass, and this would be a bug.

    @vadmium vadmium added the stdlib Python modules in the Lib dir label Jan 22, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Jan 19, 2018

    It may also be worth implementing BufferedIOBase and TextIOBase. (It seems buffering=0 isn’t reliable, e.g. rollover with limited disk space, so it may not be worth implementing RawIOBase.)

    To implement BufferedIOBase, “read1” and “readinto1” should be added.

    To implement TextIOBase, “errors” should be added, and “newlines” should only list translated newlines, not the “newline” constructor argument.

    Technically, “detach” should also be implemented, although it doesn’t have to do anything useful.

    @terryjreedy
    Copy link
    Member

    Martin, the PR has one 'approved' review, not from you. You appear to be requesting changes here, but it is not clear to me which are 'maybe' and which are 'must'.

    It is not obvious to me whether this should be treated as enhancement or behavior issue, but backporting is moot until something is merged into master.

    @terryjreedy terryjreedy added the 3.8 only security fixes label Jun 8, 2018
    @vadmium
    Copy link
    Member

    vadmium commented Jun 9, 2018

    I was making suggestions, not demanding anything. Except for the quirk with __del__, Gary’s changes (revision fb28362) look okay to add on their own as a bug fix.

    I wouldn’t claim that IOBase is “fully implemented” however, until the return values for “seek” and “truncate” are fixed.

    The underlying problem is the original SpooledTemporaryFile imitated Python 2’s file class/type, and wasn’t completely adapted for Python 3’s file classes.

    @nubirstein
    Copy link
    Mannequin

    nubirstein mannequin commented Oct 30, 2018

    Should I have been added my request there? Anyway I do suffer from lack of 'seekable()' implementation there

    @nubirstein
    Copy link
    Mannequin

    nubirstein mannequin commented Oct 30, 2018

    My last comment meant to land somewhere else, but nonetheless it is related to this topic, so:

    SpooledTemporaryFile class from lib/tempfile.py still does not implement seekable() method. It could be like this (just two lines of code and my Flask.Request tests with sending files started again to work on 3.7:

        def seekable(self):
            return self._file.seekable()

    Is it possible to add this method?

    @danieldjewell
    Copy link
    Mannequin

    danieldjewell mannequin commented May 13, 2020

    To add something additional here:

    The current documentation for tempfile.SpooledTemporaryFile indicates "This function operates exactly as TemporaryFile() does, except that data is spooled in memory until the file size exceeds max_size[...]" (see https://docs.python.org/3/library/tempfile.html)

    Except that SpooledTemporaryFile *doesn't* act _exactly_ like TemporaryFile() - as documented here. TemporaryFile() returns an "_io.BufferedRandom" which implements all of the expected "file-like" goodies - like [.readable, .seekable, ...etc]. SpooledTemporaryFile does not.

    In comparing to the 2.x docs, the text for SpooledTemporaryFile() appears to be identical or nearly identical to the 3.8.x current docs. This goes in line with what has already been discussed here.

    At a _very minimum_ the documentation should be updated to reflect the current differences between TemporaryFile() and SpooledTemporaryFile().

    Perhaps an easier change would be to extend TemporaryFile() to have a parameter that enables functionality similar to SpooledTemporaryFile? Namely, *memory-only* storage up to a max_size? Or perhaps there is an alternate solution that already exists?

    Ultimately, the functionality that appears to be missing is to be able to easily create a file-like object backed primarily by memory for reading/writing data ... (i.e. one 100% compatible with 'the usual' file objects returned by open()...)

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Oct 30, 2020

    Another test case:

    import tempfile
    import io
    import json
    
    
    with tempfile.SpooledTemporaryFile(max_size=2**20) as f:
        tf = io.TextIOWrapper(f, encoding='utf-8')
        json.dump({}, fp=tf)

    I was writing json to a file-like object that I need to read in as binary (to upload to S3). Originally the code used BytesIO and I thought it would be wise to actually spool this to disk as I was operating with possible limited RAM... except that of course it didn't work.

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Oct 30, 2020

    ... to further clarify, it is disappointing that either BytesIO or TemporaryFile would work alone, but the one that merges these two doesn't.

    @merwok
    Copy link
    Member

    merwok commented Feb 25, 2022

    I believe the PR is in good shape.
    Can someone with expertise in tempfile review it?

    It would also be useful if the people who had a bug could test the new code.

    @merwok merwok added 3.11 only security fixes and removed 3.8 only security fixes labels Feb 25, 2022
    @terryjreedy
    Copy link
    Member

    Éric, you might use git log or git blame to see who that is active has patched the relevant file in the last few years.

    @terryjreedy
    Copy link
    Member

    Also, which of the two patches.

    Irit, you just patched Temp file doc, can you look at the PR code?

    @iritkatriel
    Copy link
    Member

    Irit, you just patched Temp file doc, can you look at the PR code?

    I don't consider myself and expert here, but I left a comment on PR29560 just to be a good sport.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    methane added a commit that referenced this issue May 3, 2022
    …H-29560)
    
    Since the underlying file-like objects (either `io.BytesIO`,
    or a true file object) all implement the `io.IOBase`
    interface, the `SpooledTemporaryFile` should as well.
    
    Additionally, since the underlying file object will either be an
    instance of an `io.BufferedIOBase` (for binary mode) or an
    `io.TextIOBase` (for text mode), methods for these classes were also
    implemented.
    
    In every case, the required methods and properties are simply delegated
    to the underlying file object.
    
    Co-authored-by: Gary Fernie <Gary.Fernie@skyscanner.net>
    Co-authored-by: Inada Naoki <songofacandy@gmail.com>
    @methane methane removed the type-bug An unexpected behavior, bug, or error label May 3, 2022
    @methane methane closed this as completed May 3, 2022
    tiran added a commit to tiran/cpython that referenced this issue May 5, 2022
    ianwal added a commit to ianwal/alfbote that referenced this issue Jul 31, 2023
    SpooledTemporaryFile does not fully implement IOBase before 3.11
    python/cpython#70363
    https://bugs.python.org/issue33762
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants