Fully implement IOBase abstract on SpooledTemporaryFile

[GFernie]

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]

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)

[vadmium]

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]

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]

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.

[vadmium]

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]

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

[nubirstein]

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]

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]

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]

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

[merwok]

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.

[terryjreedy]

É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]

Also, which of the two patches.

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

[iritkatriel]

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.