Repository navigation
[python] Back-reference keeping the handler alive in stream for pyjindo impl - #10536
Conversation
sundapeng
left a comment
sundapeng
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. The crash is real: pyjindo's FileSystem.open() returns a FileStream that holds no reference to its FileSystem, and FileSystem.__dealloc__ calls destroy(). Dropping the handler while a file is open therefore frees the native store under the reader.
I reproduced this against a local OSS-compatible HTTP endpoint (pyjindosdk 6.10.4, pyarrow 19.0.1, Python 3.11). On the base 405d6d6, dropping the JindoFileSystemHandler/PyFileSystem while a file is open segfaults in all three cases I tried: reading after the eviction, reading from another thread, and releasing the unclosed file. On 2526f97 all three pass. The existing jindo_file_system_test.py also passes (11 passed, 19 skipped because no test bucket is configured).
Implementation: FINDINGS. One teardown-order issue and one ownership question are inline.
| # holds the handler (e.g. RESTTokenFileIO's TTLCache maxsize=1 | ||
| # eviction on token refresh) frees the native connection while other | ||
| # threads still read from this stream. | ||
| self._owner = owner |
There was a problem hiding this comment.
[P2] Close the stream before releasing the owner instead of relying on attribute order
The keep-alive works only because _stream is assigned before _owner. When an unclosed JindoInputFile is released, its __dict__ drops _stream first, so FileStream.__dealloc__ -> close() still runs against a live store. Nothing makes that order explicit. With the same local reproduction, assigning self._owner = owner before self._stream = jindo_stream brings the SIGSEGV back on this head when the evicted handler's last unclosed file is released. Closing the file before releasing it, or adding a __del__ that closes _stream while _owner is still set, passes with the reversed order. pyjindo's own JindoOssFile is safe for the same reason: io.IOBase closes it in its finalizer before fs is released, and the PVFS path passes the same eviction reproduction.
close() also keeps _owner, so a closed file that is still referenced pins the evicted handler and its native store longer than the stream is open. Releasing the owner after a successful close is safe: in the same reproduction, a closed file whose owner is released before its stream passes.
Please close _stream explicitly before _owner can be released and drop _owner once the close succeeds (the same applies to JindoOutputFile), and consider making owner required so a new open_* path cannot silently skip it. Please also add a regression test: the existing mock-based tests can patch jfs.connect, drop the PyFileSystem while a file is open, and assert that the stream is closed before the fake filesystem is destroyed.
| jindo_stream = self._jindo_fs.open(normalized, "rb") | ||
| return PythonFile( | ||
| JindoInputFile(jindo_stream, self._known_file_size(normalized)), | ||
| JindoInputFile(jindo_stream, self._known_file_size(normalized), owner=self), |
There was a problem hiding this comment.
Why keep the whole handler alive? The stream only depends on self._jindo_fs. Pinning the handler also retains the evicted FileIO's properties (including the AK/SK/STS token) and its _known_file_sizes cache until the file is released. owner=self._jindo_fs passes the same eviction reproduction.
- Pin only handler._jindo_fs (not the whole handler) from open streams so the evicted FileIO's credentials and size cache are released - Add __del__ finalizers closing the native stream while the owner still pins the store: attribute dealloc order is a CPython implementation detail, not a contract - Release the owner once close() succeeds so closed files no longer pin the native store - Make the owner argument keyword-only and required - Add JindoKeepAliveTest regression tests (mocked jfs, no pyjindo needed) Co-Authored-By: Claude Code <noreply@anthropic.com>
|
+1 |
4 participants
Purpose
Back-reference keeping the handler (and its native JindoSDK connection) alive: the stream is a non-owning reference into memory owned by handler._jindo_fs. Without it, dropping the FileIO that holds the handler (e.g. RESTTokenFileIO's TTLCache maxsize=1 eviction on token refresh) frees the native connection while other threads still read from this stream.