From 83d6152028ad56d7eb0e4038c9c6d5c643908629 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Wed, 30 Aug 2017 22:43:57 +0100 Subject: [PATCH 01/12] Fix SpooledTemporaryFile IOBase abstract One would assume that this class implements the IOBase abstract. As the underlying file-like object is either io.BytesIO, io.StringIO, or a true file object, this is a reasonable abstract expect and to implement. Regardless, the behaviour of this class does not change much in the case of the attribute being missing from the underlying file-like; an AttributeError is still raised, albeit from one additional frame on the stack trace. --- Lib/tempfile.py | 15 +++++++++++++++ Lib/test/test_tempfile.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 61462357c7283e..282401bef21ba4 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -685,6 +685,9 @@ def __exit__(self, exc, value, tb): def __iter__(self): return self._file.__iter__() + def __del__(self): + return self._file.__del__() + def close(self): self._file.close() @@ -737,6 +740,12 @@ def newlines(self): def read(self, *args): return self._file.read(*args) + def readable(self): + return self._file.readable() + + def readinto(self, b): + return self._file.readinto(b) + def readline(self, *args): return self._file.readline(*args) @@ -746,6 +755,9 @@ def readlines(self, *args): def seek(self, *args): self._file.seek(*args) + def seekable(self): + return self._file.seekable() + @property def softspace(self): return self._file.softspace @@ -767,6 +779,9 @@ def write(self, s): self._check(file) return rv + def writable(self): + return self._file.writable() + def writelines(self, iterable): file = self._file rv = file.writelines(iterable) diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index 710756bde64c04..fbd3ef23dba853 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -982,6 +982,35 @@ def test_basic(self): f = self.do_create(max_size=100, pre="a", suf=".txt") self.assertFalse(f._rolled) + def test_iobase_abstract(self): + # SpooledTemporaryFile should implement the IOBase abstract + iobase_abstract = ( + 'close', + 'closed', + 'fileno', + 'flush', + 'isatty', + 'read', + 'readable', + 'readinto', + 'readline', + 'readlines', + 'seek', + 'seekable', + 'tell', + 'truncate', + 'write', + 'writable', + 'writelines', + '__del__', + ) + f = self.do_create() + for attribute in iobase_abstract: + self.assertTrue( + hasattr(f, attribute), + '{} attribute missing'.format(attribute) + ) + def test_del_on_close(self): # A SpooledTemporaryFile is deleted when closed dir = tempfile.mkdtemp() From 3b69baaf336f8c3b61df2b3fae4d7a9ebc5bb1f5 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Wed, 30 Aug 2017 23:14:29 +0100 Subject: [PATCH 02/12] Add news entry for bpo-26175 patch --- .../next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst diff --git a/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst b/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst new file mode 100644 index 00000000000000..194e39c665364e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst @@ -0,0 +1,2 @@ +Fully implement `io.IOBase` abstract for `tempfile.SpooledTemporaryFile`. +Patch by Gary Fernie From 753b4fba2d70e84e8e9686d39bf6dcc162d4aa1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Araujo?= Date: Wed, 20 Sep 2017 14:10:41 -0400 Subject: [PATCH 03/12] Update 2017-08-30-23-14-17.bpo-26175.GOaj9y.rst --- .../next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst b/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst index 194e39c665364e..066ffac870da08 100644 --- a/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst +++ b/Misc/NEWS.d/next/Library/2017-08-30-23-14-17.bpo-26175.GOaj9y.rst @@ -1,2 +1,2 @@ -Fully implement `io.IOBase` abstract for `tempfile.SpooledTemporaryFile`. -Patch by Gary Fernie +Fully implement io.IOBase interface for tempfile.SpooledTemporaryFile. +Patch by Gary Fernie. From a70113ec131af4fd523da55a7ec46b9a8b170395 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sat, 30 Sep 2017 15:02:22 +0200 Subject: [PATCH 04/12] Make SpooledTemporaryFile subclass IOBase --- Lib/tempfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 282401bef21ba4..740c91edcd4818 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -626,7 +626,7 @@ def TemporaryFile(mode='w+b', buffering=-1, encoding=None, _os.close(fd) raise -class SpooledTemporaryFile: +class SpooledTemporaryFile(_io.IOBase): """Temporary file wrapper, specialized to switch from BytesIO or StringIO to a real file when it exceeds a certain size or when a fileno is needed. From fb283621685da73771256d8eb51dabf19fe71378 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sat, 30 Sep 2017 15:02:55 +0200 Subject: [PATCH 05/12] Refactor tests to more reliably test abstract --- Lib/test/test_tempfile.py | 41 +++++++++++++-------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index fbd3ef23dba853..54fbb6ded6ceff 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -982,34 +982,21 @@ def test_basic(self): f = self.do_create(max_size=100, pre="a", suf=".txt") self.assertFalse(f._rolled) - def test_iobase_abstract(self): - # SpooledTemporaryFile should implement the IOBase abstract - iobase_abstract = ( - 'close', - 'closed', - 'fileno', - 'flush', - 'isatty', - 'read', - 'readable', - 'readinto', - 'readline', - 'readlines', - 'seek', - 'seekable', - 'tell', - 'truncate', - 'write', - 'writable', - 'writelines', - '__del__', + def test_is_iobase(self): + # SpooledTemporaryFile should implement io.IOBase + self.assertIsInstance(self.do_create(), io.IOBase) + + def test_iobase_interface(self): + # SpooledTemporaryFile should implement the io.IOBase interface. + # IOBase does not declare read(), readinto(), or write(), but + # they should be considered part of the interface. + iobase_abstract = {'read', 'readinto', 'write'} + spooledtempfile_abstract = set(dir(tempfile.SpooledTemporaryFile)) + missing_attributes = iobase_abstract - spooledtempfile_abstract + self.assertFalse( + missing_attributes, + 'io.IOBase attributes missing from SpooledTemporaryFile' ) - f = self.do_create() - for attribute in iobase_abstract: - self.assertTrue( - hasattr(f, attribute), - '{} attribute missing'.format(attribute) - ) def test_del_on_close(self): # A SpooledTemporaryFile is deleted when closed From bc9d0a94a0d1c351ccb1fd9b70b7a835313c12db Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sun, 9 Sep 2018 23:49:26 +0200 Subject: [PATCH 06/12] Return new absolute position from underlying file on seek Part of file interface: https://docs.python.org/library/io.html#io.IOBase.seek --- Lib/tempfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 740c91edcd4818..7a306f6d981445 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -753,7 +753,7 @@ def readlines(self, *args): return self._file.readlines(*args) def seek(self, *args): - self._file.seek(*args) + return self._file.seek(*args) def seekable(self): return self._file.seekable() From b588dec6a80c7e113ef9fe7e22749914810f4810 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sun, 9 Sep 2018 23:50:50 +0200 Subject: [PATCH 07/12] Do nothing on __del__ We don't want to delete the underlying file explicitly as the expected behaviour is for the file to be deleted *after* it falls out of scope. --- Lib/tempfile.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 7a306f6d981445..1cae5966e65d91 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -686,7 +686,10 @@ def __iter__(self): return self._file.__iter__() def __del__(self): - return self._file.__del__() + """Do nothing. Underlying file will be deleted as a consequence + of falling out of scope, anyway, if nothing else is otherwise + deliberately referencing it.""" + pass def close(self): self._file.close() From ab49dd1494411dfd31ef5e309c17a084103e4f25 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Mon, 10 Sep 2018 00:28:57 +0200 Subject: [PATCH 08/12] Return new file size from underlying file on truncate Part of file interface: https://docs.python.org/library/io.html#io.IOBase.truncate --- Lib/tempfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 1cae5966e65d91..1d6fba87994969 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -770,11 +770,11 @@ def tell(self): def truncate(self, size=None): if size is None: - self._file.truncate() + return self._file.truncate() else: if size > self._max_size: self.rollover() - self._file.truncate(size) + return self._file.truncate(size) def write(self, s): file = self._file From 19f4c08d000f6249a99c81ebf0a1de40a22555ac Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Wed, 30 Oct 2019 19:48:00 +0100 Subject: [PATCH 09/12] Add versionchanged tag --- Doc/library/tempfile.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/Doc/library/tempfile.rst b/Doc/library/tempfile.rst index fff7a7a03eb81d..77ef5767cafb7a 100644 --- a/Doc/library/tempfile.rst +++ b/Doc/library/tempfile.rst @@ -116,6 +116,7 @@ The module defines the following user-callable items: .. versionchanged:: 3.8 Added *errors* parameter. + Implemented :py:data:`io.IOBase` inteface. .. function:: TemporaryDirectory(suffix=None, prefix=None, dir=None) From 4519b9c56ed657fe4519540f1e8b9d5fbedae268 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Wed, 30 Oct 2019 21:44:54 +0100 Subject: [PATCH 10/12] Use :class: instead of :py:data: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Éric Araujo --- Doc/library/tempfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/tempfile.rst b/Doc/library/tempfile.rst index 77ef5767cafb7a..c9822b96a14a2f 100644 --- a/Doc/library/tempfile.rst +++ b/Doc/library/tempfile.rst @@ -116,7 +116,7 @@ The module defines the following user-callable items: .. versionchanged:: 3.8 Added *errors* parameter. - Implemented :py:data:`io.IOBase` inteface. + Implemented :class:`io.IOBase` inteface. .. function:: TemporaryDirectory(suffix=None, prefix=None, dir=None) From 554aad63340c73dcf0afe0fb35ce45cf1a4341c1 Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sat, 23 Nov 2019 15:31:25 +0100 Subject: [PATCH 11/12] Revert "Do nothing on __del__" This reverts commit b588dec6a80c7e113ef9fe7e22749914810f4810. --- Lib/tempfile.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index 507aa45e69838f..eeda0d5609fc99 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -680,10 +680,7 @@ def __iter__(self): return self._file.__iter__() def __del__(self): - """Do nothing. Underlying file will be deleted as a consequence - of falling out of scope, anyway, if nothing else is otherwise - deliberately referencing it.""" - pass + return self._file.__del__() def close(self): self._file.close() From be094b38039b22c6145bee16ddf67358e44b7e8a Mon Sep 17 00:00:00 2001 From: Gary Fernie Date: Sat, 23 Nov 2019 17:08:00 +0100 Subject: [PATCH 12/12] Add test for rolled file finalisation The underlying file should be deleted when the SpooledTemporaryFile object is deleted. This follows the same behaviour as TemporaryFile and is consistent with the documentation. `__del__` should have the same consequences as using `del` or allowing the object to fall out of scope; an expectation which b588dec (reverted) broke somewhat. --- Lib/test/test_tempfile.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index f783651111d94b..8baedabe896f40 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -1016,6 +1016,21 @@ def test_del_on_close(self): finally: os.rmdir(dir) + def test_del_rolled_file(self): + # The rolled file should be deleted when the + # SpooledTemporaryFile object is deleted. This should raise a + # ResourceWarning since the file was not explicitly closed. + f = self.do_create(max_size=2) + f.write(b'foo') + filename = f.name + self.assertTrue(os.path.exists(filename)) + with self.assertWarns(ResourceWarning): + f.__del__() + self.assertFalse( + os.path.exists(filename), + "Rolled SpooledTemporaryFile not deleted after __del__" + ) + def test_rewrite_small(self): # A SpooledTemporaryFile can be written to multiple within the max_size f = self.do_create(max_size=30)