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.
Please close `_stream` explicitly before `_owner` can be released (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.
##########
paimon-python/pypaimon/filesystem/jindo_file_system_handler.py:
##########
@@ -373,20 +383,20 @@ def open_input_stream(self, path: str):
normalized = self._normalize_path(path)
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),
Review 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.
--
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]
