Closing a ZstdCompressionWriter or ZstdDecompressionWriter should not close underlying file

[rtkbkish]

The changes to support ZstdCompressionWriter and ZstdDecompressionWriter without a context mananger has broken use cases where they are being used in a context manager.

Upon a close, the writers should not close their underlying file objects. It should be up to the caller to close the file objects.

We are passing in StringIO objects to the writers and they get closed upon exit--throwing out the buffer. This technique works with other compression libraries.

[indygreg]

The behavior of stream wrappers and close() isn't very well-defined by the Python docs and I had trouble understanding what the expected behavior should be. In fact, there is a TODO item to audit the behavior and ensure consistency.

I'm pretty sure I found at least one example (io.BufferedReader) where close() does in fact close() the underlying stream. Maybe that behavior only makes sense on readers instead of writers?

I understand your pain here. I'd prefer to change things once. And that change should match the behavior of other stream wrappers in Python. So I'll need to do some homework.

Specifically:

1. Does a stream wrapper call close() on the underlying stream when close() is called?
2. Does a stream wrapper call close() on the underlying stream when __exit__ is called?
3. If 2 is yes, does behavior change if an exception was raised or does the main/underlying stream always get closed during __exit__?
4. Does it matter if the stream wrapper is a reader or writer?

[indygreg]

I should mention that a workaround would be to use writer.flush(zstd.FLUSH_FRAME). This will ensure a full zstd frame is flushed to the underlying stream without closing the stream. close() at this point shouldn't perform any I/O other than to close the underlying stream.

[rtkbkish]

https://github.com/python/cpython/blob/master/Lib/gzip.py

    def close(self):
        fileobj = self.fileobj
        if fileobj is None:
            return
        self.fileobj = None
        try:
            if self.mode == WRITE:
                fileobj.write(self.compress.flush())
                write32u(fileobj, self.crc)
                # self.size may exceed 2 GiB, or even 4 GiB
                write32u(fileobj, self.size & 0xffffffff)
            elif self.mode == READ:
                self._buffer.close()
        finally:
            myfileobj = self.myfileobj
            if myfileobj:
                self.myfileobj = None
                myfileobj.close()

fileobj is the output object
myfileobj is None if passed in an output object, or it equals fileobj if it opened the file itself.

fileobj is not closed
myfileobj, if it exists, is closed

[indygreg]

I've filed https://bugs.python.org/issue36129 to seek clarification from upstream.

[ghannum]

I'm having a similar issue with zstd.ZstdDecompressor().stream_reader not closing the inner file when an outer TextIOWrapper closes. Can this also be addressed?

[indygreg]

Thanks for the report, @ghannum. While I think I know what you are saying, would you mind providing a use case showing the behavior and commenting exactly where you think things are misbehaving?

I'm wondering if we're getting into trouble because we don't involve close() when destroying the stream wrappers. i.e. we're missing a proper __del__ implementation (https://docs.python.org/3/library/io.html#io.IOBase.__del__).

[ghannum]

   fh = open("test.zst", 'rb')
   f = io.TextIOWrapper(zstd.ZstdDecompressor().stream_reader(fh)) 
   f.close()

   print(fh.closed)

False

[ghannum]

@indygreg any update on this?

[indygreg]

The reply to https://bugs.python.org/issue36129 says that flush() and close() should cascade. However, the compression *File types do not do this. It sounds like a closefd argument can be used to control this behavior. I'm waiting for clarification that this is the case. If closefd should apply to any file object, then we should add a closefd argument to our stream wrapping types and have it default to True. The default would be inconsistent with the compression *File types in the Python standard library. But it would be consistent with the ideal that close() cascades by default. I do not want to cargo cult standard library bugs into python-zstandard if it can be avoided.

[ghannum]

Thanks for the response. It is an interesting question I hadn't thought of.

Looking at the Python standard libraries, it seems this sort of control is assumed through the parameter type - ie. If you pass gzip.open a steam, it will not cascade a 'close' event (just borrowing the steam). But if you pass it a filename, it will handle all of the file-associated actions, even cascading the close through a hidden TextIOWrapper if requested (creating and owning the file handle and text wrapper).

If the goal is to maintain the Python conventions, then perhaps a similar zstd.open function would be appropriate?

[indygreg]

I do plan to provide an open() function at some time. I thought about doing it for the last release. But I wanted the stream_reader() and stream_writer() APIs to be a bit more stable first. Now that the zstandard module is backed by a .py file instead of an extension, it should be trivial to implement higher-level functionality like open(). (Another feature will likely be a function to open a tar file with zstd compression.)

[ghannum]

Awesome. I'm looking forward to it!

[dholth]

Is this the same bug? The nested zip file is corrupt when extracted.

(oh, it may be that ZipFile simply doesn't close the underlying file when it is a stream and not a filename)

compression = zstandard.ZstdCompressor(level=10)
z1 = ZipFile('a.zip', 'w')
writer = z1.open('nested.zip.zst', 'w')
compressed = compression.stream_writer(writer)
z2 = zipfile.ZipFile(compressed, mode="w", compression=zipfile.ZIP_STORED)
z2.open(...).write(...)

[indygreg]

I just pushed some commits to add closefd arguments to the stream_reader() and stream_writer() methods for constructing IO objects. The writers default to closefd=True and the readers to closefd=False. I may change these defaults before the next release so behavior is consistent.

[indygreg]

I'm about to close this issue via a pushed commit. The state of the API will be as follows:

• All stream_reader() and stream_writer() methods that construct a file-like object accept a closefd boolean argument that controls whether to close() the passed stream when the instance itself is closed.
• closefd defaults to True in all cases, meaning close() cascades by default.
• __exit__ will always call self.close().

While the issue summary requests that closefd=False, this goes against the suggestion from upstream that close() should cascade. I'm inclined to go with upstream's suggestion here. And since there is a closefd argument to explicitly control behavior, if you want different behavior you can simply change that argument.