sundapeng commented on code in PR #10536:
URL: https://github.com/apache/paimon/pull/10536#discussion_r4256350084
##########
paimon-python/pypaimon/filesystem/jindo_file_system_handler.py:
##########
@@ -131,9 +131,16 @@ def create_jindo_oss_filesystem(root_uri: str,
catalog_options: Options):
class JindoInputFile:
- def __init__(self, jindo_stream, file_size=None):
+ def __init__(self, jindo_stream, file_size=None, owner=None):
self._stream = jindo_stream
self._file_size = file_size
+ # 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.
+ self._owner = owner
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
