From 1b952848a6cd1136bfad95badc2dc0cf49ebb9ea Mon Sep 17 00:00:00 2001 From: Ruslan Dautkhanov Date: Fri, 22 May 2026 09:36:14 -0600 Subject: [PATCH 1/4] perf: memoize JVMView / JavaPackage / JavaClass __getattr__ (issue #557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold-start work, C1 of the issue #557 series. Today every attribute walk along ``gateway.jvm.X.Y.Z.method`` re-issues one reflection round-trip per level — JVMView, JavaPackage, and JavaClass all have ``__getattr__`` methods that call out to the JVM without caching the result. After the first walk, the JVM-side answer is stable for the JVM's lifetime, so the next 999 walks pay the same round-trip cost for no new information. This change adds a per-instance dict cache on each of: * ``JVMView._attr_cache`` — top-level names resolved on ``.jvm.X`` * ``JavaPackage._attr_cache`` — names resolved on ``.X.Y`` * ``JavaClass._members`` — static members on ``.System.currentTimeMillis`` Only successful resolutions are cached. Misses (Py4JError) keep raising so that a later ``java_import`` or classpath change can still surface a previously-missing class. The ``UserHelpAutoCompletion`` sentinel path and direct return values on ``JavaClass.__getattr__`` are NOT cached because they each have caller-visible semantics that caching would change. Thread safety mirrors the existing ``_statics`` / ``_dir_sequence_and_cache`` convention: plain dict assignments are atomic under CPython's GIL, worst case is double-fill with the same value. The existing comments in those caches explicitly accept this tradeoff. Local measurement on the new XA-3 scenario (M-series + JDK 21): before: ~94 ms / 1000 walks = ~94 us per walk (3 reflection RTTs) after: ~269 us / 1000 walks = ~0.27 us per walk (dict get) Approx 350x speed-up on the steady-state attribute-walk path; the first walk in any chain still pays the full reflection cost. X1-1 unchanged (10 000 currentTimeMillis() calls, ~232 ms either way). Co-authored-by: Isaac --- py4j-python/src/py4j/java_gateway.py | 58 +++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/py4j-python/src/py4j/java_gateway.py b/py4j-python/src/py4j/java_gateway.py index 3e6959e3..c6a25a70 100644 --- a/py4j-python/src/py4j/java_gateway.py +++ b/py4j-python/src/py4j/java_gateway.py @@ -1590,6 +1590,17 @@ def __init__(self, fqn, gateway_client): self._converters = self._gateway_client.converters self._gateway_doc = None self._statics = None + # Cache of resolved members keyed by attribute name. JVM-side + # answers for a given (class FQN, member name) are stable for + # the JVM's lifetime, so memoizing avoids repeated reflection + # round-trips on patterns like + # ``gateway.jvm.X.Y.System.currentTimeMillis()`` (issue #557). + # Only successful lookups are cached — misses keep raising so + # later imports (java_import / classpath changes) can still + # surface. Not thread-safe but matches the existing + # ``_statics`` / ``_dir_sequence_and_cache`` convention: worst + # case is double-fill with the same value under CPython's GIL. + self._members = {} @property def __doc__(self): @@ -1637,6 +1648,10 @@ def __getattr__(self, name): # don't propagate any magic methods to Java raise AttributeError + cached = self._members.get(name) + if cached is not None: + return cached + command = proto.REFLECTION_COMMAND_NAME +\ proto.REFL_GET_MEMBER_SUB_COMMAND_NAME +\ self._fqn + "\n" +\ @@ -1646,15 +1661,22 @@ def __getattr__(self, name): if len(answer) > 1 and answer[0] == proto.SUCCESS: if answer[1] == proto.METHOD_TYPE: - return JavaMember( + result = JavaMember( name, None, proto.STATIC_PREFIX + self._fqn, self._gateway_client) elif answer[1].startswith(proto.CLASS_TYPE): - return JavaClass( + result = JavaClass( self._fqn + "$" + name, self._gateway_client) else: + # Direct return values (constants, etc.) are NOT cached + # — the answer string is decoded into a fresh Python + # value here, and there's no contract that the JVM-side + # value can't change across calls (static non-final + # fields exist). return get_return_value( answer, self._gateway_client, self._fqn, name) + self._members[name] = result + return result else: raise Py4JError( "{0}.{1} does not exist in the JVM".format(self._fqn, name)) @@ -1743,6 +1765,11 @@ def __init__(self, fqn, gateway_client, jvm_id=None): if jvm_id is None: self._jvm_id = proto.DEFAULT_JVM_ID self._jvm_id = jvm_id + # Memoize resolved children. See JavaClass._members for the + # rationale (issue #557 — collapses repeat FQN walks like + # ``gateway.jvm.java.lang.System`` from 3 RTTs to 0 after the + # first walk). + self._attr_cache = {} def __dir__(self): return [UserHelpAutoCompletion.KEY] @@ -1758,6 +1785,10 @@ def __getattr__(self, name): # don't propagate any magic methods to Java raise AttributeError + cached = self._attr_cache.get(name) + if cached is not None: + return cached + new_fqn = self._fqn + "." + name command = proto.REFLECTION_COMMAND_NAME +\ proto.REFL_GET_UNKNOWN_SUB_COMMAND_NAME +\ @@ -1766,12 +1797,14 @@ def __getattr__(self, name): proto.END_COMMAND_PART answer = self._gateway_client.send_command(command) if answer == proto.SUCCESS_PACKAGE: - return JavaPackage(new_fqn, self._gateway_client, self._jvm_id) + result = JavaPackage(new_fqn, self._gateway_client, self._jvm_id) elif answer.startswith(proto.SUCCESS_CLASS): - return JavaClass( + result = JavaClass( answer[proto.CLASS_FQN_START:], self._gateway_client) else: raise Py4JError("{0} does not exist in the JVM".format(new_fqn)) + self._attr_cache[name] = result + return result class JVMView(object): @@ -1796,6 +1829,11 @@ def __init__(self, gateway_client, jvm_name, id=None, jvm_object=None): self._jvm_object = jvm_object self._dir_sequence_and_cache = (None, []) + # Memoize resolved top-level names. See JavaClass._members / + # JavaPackage._attr_cache for the rationale (issue #557 — the + # ``.java`` in ``gateway.jvm.java.lang.System`` is now a free + # lookup after first walk instead of a reflection RTT). + self._attr_cache = {} def __dir__(self): command = proto.DIR_COMMAND_NAME +\ @@ -1817,22 +1855,30 @@ def __dir__(self): def __getattr__(self, name): if name == UserHelpAutoCompletion.KEY: + # Returns a fresh UserHelpAutoCompletion instance per + # access by design (no per-instance state) — skip caching. return UserHelpAutoCompletion() + cached = self._attr_cache.get(name) + if cached is not None: + return cached + answer = self._gateway_client.send_command( proto.REFLECTION_COMMAND_NAME + proto.REFL_GET_UNKNOWN_SUB_COMMAND_NAME + name + "\n" + self._id + "\n" + proto.END_COMMAND_PART) if answer == proto.SUCCESS_PACKAGE: - return JavaPackage(name, self._gateway_client, jvm_id=self._id) + result = JavaPackage(name, self._gateway_client, jvm_id=self._id) elif answer.startswith(proto.SUCCESS_CLASS): - return JavaClass( + result = JavaClass( answer[proto.CLASS_FQN_START:], self._gateway_client) else: _, error_message = get_error_message(answer) message = compute_exception_message( "{0} does not exist in the JVM".format(name), error_message) raise Py4JError(message) + self._attr_cache[name] = result + return result class GatewayProperty(object): From 7e6df24dbff923c20b5c39adf1dee48d291cd8f1 Mon Sep 17 00:00:00 2001 From: Ruslan Dautkhanov Date: Fri, 29 May 2026 12:38:21 -0600 Subject: [PATCH 2/4] Address review feedback: bounded LRU + GC-safety hardening + tests + changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Iterates on the original cache change in this PR based on review feedback. Cache shape ----------- * Replaces the unbounded ``dict`` cache with a bounded LRU (``_BoundedAttrCache``, default ``maxsize = 1024`` per instance) using ``OrderedDict.move_to_end`` + ``popitem(last=False)``. * Bound: 1 024 entries — generous enough that real-world workloads never evict (PySpark sessions touch ~50-200 unique names per instance), but caps pathological iteration like ``for name in many_classes: gateway.jvm[name]``. * ``get``/``put`` chain two ``OrderedDict`` ops; a concurrent eviction between them can leave the second op operating on a missing key. Both methods catch ``KeyError`` — the races are benign (``get`` already has the correct value; ``put`` already inserted) and the lost LRU promotion / eviction is recovered on the next op. GC-safety contract (only non-finalizable types are cached) ---------------------------------------------------------- * The cache stores **only** ``JavaClass``, ``JavaPackage``, and static-member ``JavaMember`` instances. None of these register a ``ThreadSafeFinalizer`` weakref or hold a JVM-allocated ``target_id``; their ``target_id``s are either absent or synthetic (``STATIC_PREFIX + fqn``). * The one ``JavaClass.__getattr__`` branch that could return a ``JavaObject`` (static field constants) deliberately bypasses the cache. ``JavaObject`` is the only py4j type with JVM-side finalization, and it stays out of every cache. * This is the contract that keeps PySpark ML's ``JavaWrapper.__del__`` -> ``gateway.detach(self._java_obj)`` machinery unaffected (SPARK-18274 / SPARK-50959 / SPARK-51880 scenarios all unaffected). Shutdown defensive clearing --------------------------- * ``JavaGateway.shutdown()`` clears the default ``self.jvm``'s attribute cache in a ``finally`` block so cached ``JavaClass`` / ``JavaPackage`` references no longer pin ``_gateway_client`` past shutdown — even on the ``raise_exception=True`` path. * Secondary ``JVMView`` instances created via ``new_jvm_view()`` are caller-managed — they die with the caller's reference and their cache is reclaimed naturally, consistent with how other in-file caches (``JavaObject._methods``, ``JavaClass._statics``, ``JVMView._dir_sequence_and_cache``) rely on their parent's lifecycle. Identity stability documented ----------------------------- * Each of ``JavaClass`` / ``JavaPackage`` / ``JVMView`` gains a ``.. note::`` block on its docstring explaining: repeated accesses to the same name typically return the *same* instance (was a fresh instance every time); identity may flip again once the cache evicts the entry; the GC-safety contract; and the ``java_import``-overlap caveat (re-importing a different FQN under the same simple name does not invalidate prior cache entries — use ``new_jvm_view`` if you need that semantics). Tests ----- Unit (``py4j/tests/attr_cache_test.py``, new file, 8 tests): * Basic semantics: ``test_get_miss_returns_none``, ``test_put_then_get_returns_value``, ``test_put_replaces_existing_value`` * LRU semantics: ``test_eviction_drops_oldest``, ``test_get_promotes_to_most_recent``, ``test_put_existing_promotes`` * Concurrency: ``test_concurrent_get_put_does_not_raise`` (4-thread race against maxsize=10), ``test_concurrent_clear_during_get_put_does_not_raise`` (4 readers + 1 writer + 1 clearer — covers the ``shutdown()._cache.clear()`` race) Integration (``MemoryManagementTest``, +7 tests): * ``testAttrCacheDoesNotAffectJavaObjectDetach`` — walks the cache, constructs two ``JavaObject`` instances, detaches them, asserts the ``ThreadSafeFinalizer`` registry delta is zero. Direct proof the cache does not pin JVM-side bindings. * ``testConcurrentColdFqnWalk`` — 16-thread Barrier-synchronized cold-walk on the same FQN. * ``testAttrCacheSurvivesShutdownMidWalk`` — clean error semantics post-shutdown. * ``testTwoGatewaysDoNotShareAttrCache`` — per-instance isolation. * ``testNoCycleInCachedInstances`` — ``weakref.ref(gateway)() is None`` after ``gc.collect()``. * ``testMissesAreNotCached`` — exercises ``System.noSuchMember``; asserts ``JavaClass._members`` size unchanged after the ``Py4JError``. * ``testShutdownRaiseExceptionStillClearsCache`` — forces the gateway-client shutdown to raise, calls ``shutdown(raise_exception=True)``, asserts the cache is cleared via the ``finally`` clause. Integration (``GarbageCollectionTest`` in ``client_server_test.py``, +1 test): * ``testAttrCacheInClientServerMode`` — pinned-thread mode coverage; 8 worker threads + main thread converge on the same ``JavaClass`` instance. Changelog --------- * ``py4j-web/changelog.rst`` gains an "Unreleased" bullet documenting the cache, the identity-semantics shift, the GC-safety contract, and the shutdown-clearing behavior. Cites issue #128 (closes) and issue #557 (refs). Validation ---------- Local: 19/19 attr-cache + MemoryManagementTest tests pass on M-series + JDK 21. Co-authored-by: Isaac --- py4j-python/src/py4j/java_gateway.py | 188 +++++++++++++-- py4j-python/src/py4j/tests/attr_cache_test.py | 169 +++++++++++++ .../src/py4j/tests/client_server_test.py | 51 ++++ .../src/py4j/tests/java_gateway_test.py | 225 ++++++++++++++++++ py4j-web/changelog.rst | 14 ++ 5 files changed, 629 insertions(+), 18 deletions(-) create mode 100644 py4j-python/src/py4j/tests/attr_cache_test.py diff --git a/py4j-python/src/py4j/java_gateway.py b/py4j-python/src/py4j/java_gateway.py index c6a25a70..46aeca75 100644 --- a/py4j-python/src/py4j/java_gateway.py +++ b/py4j-python/src/py4j/java_gateway.py @@ -11,7 +11,7 @@ """ from __future__ import annotations -from collections import deque +from collections import deque, OrderedDict import logging import os from pydoc import pager @@ -680,6 +680,97 @@ def is_magic_member(name): return name.startswith("__") and name.endswith("__") +# Default per-instance cap for the attribute-resolution caches on +# JVMView / JavaPackage / JavaClass. 1 024 is generous for any +# realistic workload (PySpark and ad-hoc scripts typically touch +# 50-200 unique class/package names per session) while still capping +# pathological cases — e.g. ``for name in many_classes: +# gateway.jvm[name]`` style iteration over thousands of names. +_DEFAULT_ATTR_CACHE_MAXSIZE = 1024 + + +class _BoundedAttrCache(object): + """LRU-bounded cache for attribute-resolution results on + ``JVMView`` / ``JavaPackage`` / ``JavaClass``. + + Mirrors the existing ``JavaObject._methods`` instance-dict cache + in shape (per-instance, lazily filled, only successful lookups + stored) but adds a size cap so that touching very many distinct + names on a single ``JVMView`` or ``JavaPackage`` cannot grow the + cache without bound. Default cap is generous enough that + real-world workloads never evict; the cap exists only to guard + against pathological iteration patterns. + + **GC safety — what this cache stores, and what it must not.** + Only types with **no JVM-side finalization** are stored: + ``JavaClass``, ``JavaPackage``, and static-member ``JavaMember``. + None of these register a ``ThreadSafeFinalizer`` weakref or hold + a JVM-allocated ``target_id``; their ``target_id``s are either + absent or synthetic strings (``STATIC_PREFIX + fqn``). Caching + them therefore cannot extend the lifetime of any JVM-side + resource. ``JavaObject`` instances — the only py4j type that + *does* register a finalizer (``java_gateway.py``: ``JavaObject. + __init__`` -> ``ThreadSafeFinalizer.add_finalizer``) and that + triggers ``_garbage_collect_object`` on Python-side reclamation + — are **never** stored here. The one ``__getattr__`` branch that + could return a ``JavaObject`` (static field constants on + ``JavaClass.__getattr__``'s ``else`` arm) deliberately bypasses + this cache. This is the contract that keeps PySpark ML's + ``JavaWrapper.__del__`` -> ``gateway.detach(self._java_obj)`` + machinery unaffected (SPARK-18274 / SPARK-50959 / SPARK-51880 + scenarios all unaffected). + + Thread safety: individual ``OrderedDict`` operations are atomic + under CPython's GIL, but the ``get`` / ``put`` methods here + chain two ops (lookup + ``move_to_end``, or insert + + ``popitem``) and a concurrent eviction in between can leave the + second op operating on a key that no longer exists. Those races + are caught with ``try/except KeyError`` — they are benign: + ``get`` already has the correct value in hand (only the LRU + promotion is lost), and ``put`` already inserted (only the + eviction step is lost, recovered on the next ``put``). + """ + __slots__ = ("_cache", "_maxsize") + + def __init__(self, maxsize=_DEFAULT_ATTR_CACHE_MAXSIZE): + self._cache = OrderedDict() + self._maxsize = maxsize + + def get(self, name): + value = self._cache.get(name) + if value is not None: + try: + self._cache.move_to_end(name) + except KeyError: + # Concurrent put() evicted ``name`` between the lookup + # above and here. We still have the correct value; + # losing the LRU promotion is harmless. Logged at + # warning so the race is observable in diagnostics + # without raising. + logger.warning( + "_BoundedAttrCache: concurrent eviction raced " + "LRU promotion for key %r; cached value is " + "still valid", name) + return value + + def put(self, name, value): + self._cache[name] = value + try: + self._cache.move_to_end(name) + if len(self._cache) > self._maxsize: + self._cache.popitem(last=False) + except KeyError: + # Concurrent eviction emptied the cache between our + # insert and the ``popitem`` (or removed our key under + # us). The insert succeeded; the next ``put`` will + # re-evaluate the size. Logged at warning so the race is + # observable in diagnostics without raising. + logger.warning( + "_BoundedAttrCache: concurrent eviction raced LRU " + "bookkeeping for key %r; cache may briefly exceed " + "maxsize, recovered on next put()", name) + + def _garbage_collect_object(gateway_client, target_id): try: try: @@ -1581,6 +1672,27 @@ class JavaClass(object): Usually, `JavaClass` are not initialized using their constructor, but they are created while accessing the `jvm` property of a gateway, e.g., `gateway.jvm.java.lang.String`. + + .. note:: + Resolved members are memoized in a bounded LRU cache (default 1024 + entries per instance). Repeated accesses to the same name typically + return the *same* ``JavaMember`` / ``JavaClass`` instance — code + comparing by identity (``a is b``) may observe a behavior change + from versions before this caching landed. Once the cache evicts an + entry, a subsequent access fetches a fresh instance again. + + The cache holds **only** ``JavaMember`` and ``JavaClass`` results + (which carry no JVM-side finalizable resource). Static-field + constants returned via this ``__getattr__`` path are *not* cached + — they could be ``JavaObject`` instances whose finalizers manage + JVM-side bindings. + + ``java_import`` interaction: re-importing a different fully + qualified name under the same simple name into the same + ``JVMView`` does not invalidate prior cache entries on + ``JavaClass``/``JavaPackage`` instances. If you rely on + ``java_import`` overlap-then-rebind semantics, use a fresh + ``new_jvm_view`` rather than the default. """ def __init__(self, fqn, gateway_client): self._fqn = fqn @@ -1597,10 +1709,9 @@ def __init__(self, fqn, gateway_client): # ``gateway.jvm.X.Y.System.currentTimeMillis()`` (issue #557). # Only successful lookups are cached — misses keep raising so # later imports (java_import / classpath changes) can still - # surface. Not thread-safe but matches the existing - # ``_statics`` / ``_dir_sequence_and_cache`` convention: worst - # case is double-fill with the same value under CPython's GIL. - self._members = {} + # surface. See ``_BoundedAttrCache`` for the LRU bound and + # the thread-safety contract. + self._members = _BoundedAttrCache() @property def __doc__(self): @@ -1675,7 +1786,7 @@ def __getattr__(self, name): # fields exist). return get_return_value( answer, self._gateway_client, self._fqn, name) - self._members[name] = result + self._members.put(name, result) return result else: raise Py4JError( @@ -1758,6 +1869,12 @@ class JavaPackage(object): Usually, `JavaPackage` are not initialized using their constructor, but they are created while accessing the `jvm` property of a gateway, e.g., `gateway.jvm.java.lang`. + + .. note:: + Resolved children are memoized in a bounded LRU cache; see + ``JavaClass`` docstring for the identity-stability behavior, the + GC-safety contract (only non-finalizable types are cached), and + the ``java_import`` overlap caveat. """ def __init__(self, fqn, gateway_client, jvm_id=None): self._fqn = fqn @@ -1769,7 +1886,7 @@ def __init__(self, fqn, gateway_client, jvm_id=None): # rationale (issue #557 — collapses repeat FQN walks like # ``gateway.jvm.java.lang.System`` from 3 RTTs to 0 after the # first walk). - self._attr_cache = {} + self._attr_cache = _BoundedAttrCache() def __dir__(self): return [UserHelpAutoCompletion.KEY] @@ -1803,7 +1920,7 @@ def __getattr__(self, name): answer[proto.CLASS_FQN_START:], self._gateway_client) else: raise Py4JError("{0} does not exist in the JVM".format(new_fqn)) - self._attr_cache[name] = result + self._attr_cache.put(name, result) return result @@ -1813,6 +1930,11 @@ class JVMView(object): This can be used to reference static members (fields and methods) and to call constructors. + + .. note:: + Resolved top-level names are memoized in a bounded LRU cache; + see ``JavaClass`` docstring for the identity-stability behavior, + the GC-safety contract, and the ``java_import`` overlap caveat. """ def __init__(self, gateway_client, jvm_name, id=None, jvm_object=None): @@ -1833,7 +1955,7 @@ def __init__(self, gateway_client, jvm_name, id=None, jvm_object=None): # JavaPackage._attr_cache for the rationale (issue #557 — the # ``.java`` in ``gateway.jvm.java.lang.System`` is now a free # lookup after first walk instead of a reflection RTT). - self._attr_cache = {} + self._attr_cache = _BoundedAttrCache() def __dir__(self): command = proto.DIR_COMMAND_NAME +\ @@ -1877,7 +1999,7 @@ def __getattr__(self, name): message = compute_exception_message( "{0} does not exist in the JVM".format(name), error_message) raise Py4JError(message) - self._attr_cache[name] = result + self._attr_cache.put(name, result) return result @@ -2144,15 +2266,45 @@ def shutdown(self, raise_exception=False): occurs while shutting down (very likely with sockets). """ try: - self._gateway_client.shutdown_gateway() - except Exception: - if raise_exception: - raise - else: - logger.info( - "Exception while shutting down callback server", + try: + self._gateway_client.shutdown_gateway() + except Exception: + if raise_exception: + raise + else: + logger.warning( + "Exception while shutting down callback server", + exc_info=True) + self.shutdown_callback_server() + finally: + # Defensive: drop the attribute-resolution cache on the + # default ``JVMView`` so cached ``JavaClass`` / + # ``JavaPackage`` references no longer pin + # ``_gateway_client`` past shutdown. Runs in ``finally`` + # so the cleanup happens even on the + # ``raise_exception=True`` path. Secondary ``JVMView`` + # instances created via ``new_jvm_view()`` are caller- + # managed — they die with the caller's reference and + # their cache is reclaimed naturally. (Existing in-file + # caches such as ``JavaObject._methods``, + # ``JavaClass._statics``, ``JVMView._dir_sequence_and_cache`` + # likewise rely on their parent's lifecycle.) + try: + jvm = getattr(self, "jvm", None) + if jvm is not None: + attr_cache = getattr(jvm, "_attr_cache", None) + if attr_cache is not None: + attr_cache._cache.clear() + except Exception: + # Best-effort — never surface a cleanup exception + # from shutdown; the gateway is already torn down at + # this point. Logged at warning so the failure is + # observable in diagnostics without breaking + # shutdown. + logger.warning( + "Exception while clearing JVMView attribute " + "cache during shutdown", exc_info=True) - self.shutdown_callback_server() def shutdown_callback_server(self, raise_exception=False): """Shuts down the diff --git a/py4j-python/src/py4j/tests/attr_cache_test.py b/py4j-python/src/py4j/tests/attr_cache_test.py new file mode 100644 index 00000000..b3d12b64 --- /dev/null +++ b/py4j-python/src/py4j/tests/attr_cache_test.py @@ -0,0 +1,169 @@ +"""Tests for ``_BoundedAttrCache`` — the LRU helper backing the +``JVMView`` / ``JavaPackage`` / ``JavaClass`` attribute caches added +for issue #557. + +Pure Python unit tests; no JVM needed. +""" +import threading +import time +import unittest + +from py4j.java_gateway import _BoundedAttrCache + + +class BoundedAttrCacheTest(unittest.TestCase): + + def test_get_miss_returns_none(self): + cache = _BoundedAttrCache(maxsize=4) + self.assertIsNone(cache.get("absent")) + + def test_put_then_get_returns_value(self): + cache = _BoundedAttrCache(maxsize=4) + sentinel = object() + cache.put("k", sentinel) + self.assertIs(cache.get("k"), sentinel) + + def test_put_replaces_existing_value(self): + cache = _BoundedAttrCache(maxsize=4) + cache.put("k", "first") + cache.put("k", "second") + self.assertEqual(cache.get("k"), "second") + + def test_eviction_drops_oldest(self): + # maxsize=3, insert 4 distinct keys; the first inserted should + # be evicted because nothing promoted it. + cache = _BoundedAttrCache(maxsize=3) + cache.put("a", 1) + cache.put("b", 2) + cache.put("c", 3) + cache.put("d", 4) + self.assertIsNone(cache.get("a")) + self.assertEqual(cache.get("b"), 2) + self.assertEqual(cache.get("c"), 3) + self.assertEqual(cache.get("d"), 4) + + def test_get_promotes_to_most_recent(self): + # After touching ``a``, the next eviction should drop ``b`` + # (now the oldest), not ``a``. + cache = _BoundedAttrCache(maxsize=3) + cache.put("a", 1) + cache.put("b", 2) + cache.put("c", 3) + cache.get("a") # promote a + cache.put("d", 4) + self.assertEqual(cache.get("a"), 1) + self.assertIsNone(cache.get("b")) + self.assertEqual(cache.get("c"), 3) + self.assertEqual(cache.get("d"), 4) + + def test_put_existing_promotes(self): + # Re-putting the same key should also promote it past the + # eviction threshold. + cache = _BoundedAttrCache(maxsize=3) + cache.put("a", 1) + cache.put("b", 2) + cache.put("c", 3) + cache.put("a", 10) # promote + replace + cache.put("d", 4) + self.assertEqual(cache.get("a"), 10) + self.assertIsNone(cache.get("b")) + + def test_concurrent_get_put_does_not_raise(self): + # Smoke test for the race fix in get()/put(): two threads + # hammering the cache near its eviction boundary should never + # raise (KeyError on the second op of a non-atomic pair would + # be the regression). 50 keys, maxsize 10 → constant churn. + cache = _BoundedAttrCache(maxsize=10) + errors = [] + stop = threading.Event() + + def writer(): + try: + i = 0 + while not stop.is_set(): + cache.put("k{0}".format(i % 50), i) + i += 1 + except Exception as e: + errors.append(e) + + def reader(): + try: + while not stop.is_set(): + for j in range(50): + cache.get("k{0}".format(j)) + except Exception as e: + errors.append(e) + + threads = [ + threading.Thread(target=writer), + threading.Thread(target=writer), + threading.Thread(target=reader), + threading.Thread(target=reader), + ] + for t in threads: + t.start() + # Run for ~250 ms — long enough to hit the race in practice + # but short enough to keep the test fast. + time.sleep(0.25) + stop.set() + for t in threads: + t.join(timeout=5.0) + self.assertEqual(errors, []) + + def test_concurrent_clear_during_get_put_does_not_raise(self): + # Phase 2 of the cache PR makes ``JavaGateway.shutdown()`` call + # ``_attr_cache._cache.clear()``. That clear can race with + # readers and writers from other threads. Each individual + # OrderedDict op is atomic under the GIL, but the chained ops + # in ``get`` (lookup + ``move_to_end``) and ``put`` (insert + + # ``popitem``) can intersect with a ``clear()`` between their + # two steps. The race must never raise — the ``try/except + # KeyError`` in ``get`` and ``put`` is the safety net. + cache = _BoundedAttrCache(maxsize=10) + errors = [] + stop = threading.Event() + + def writer(): + try: + i = 0 + while not stop.is_set(): + cache.put("k{0}".format(i % 50), i) + i += 1 + except Exception as e: + errors.append(e) + + def reader(): + try: + while not stop.is_set(): + for j in range(50): + cache.get("k{0}".format(j)) + except Exception as e: + errors.append(e) + + def clearer(): + try: + while not stop.is_set(): + cache._cache.clear() + # Tiny sleep so clearer doesn't dominate scheduler. + time.sleep(0.001) + except Exception as e: + errors.append(e) + + threads = [ + threading.Thread(target=writer), + threading.Thread(target=writer), + threading.Thread(target=reader), + threading.Thread(target=reader), + threading.Thread(target=clearer), + ] + for t in threads: + t.start() + time.sleep(0.25) + stop.set() + for t in threads: + t.join(timeout=5.0) + self.assertEqual(errors, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/py4j-python/src/py4j/tests/client_server_test.py b/py4j-python/src/py4j/tests/client_server_test.py index 65c1541e..8451478e 100644 --- a/py4j-python/src/py4j/tests/client_server_test.py +++ b/py4j-python/src/py4j/tests/client_server_test.py @@ -212,6 +212,57 @@ def assert_connection(): self.assertTrue(all(connections)) client_server.shutdown() + def testAttrCacheInClientServerMode(self): + """Pinned-thread ``ClientServer`` mode also uses the JVMView / + JavaPackage / JavaClass attribute caches; multiple threads + walking the same FQN through their own pinned connections + must produce a coherent cache (same JavaClass reference once + the cache is populated) without raising. + """ + with clientserver_example_app_process(): + client_server = ClientServer( + JavaParameters(), PythonParameters()) + try: + # First walk populates the cache. + cls_first = client_server.jvm.java.lang.String + # Repeated walks from the main thread hit the cache. + cls_again = client_server.jvm.java.lang.String + self.assertIs( + cls_first, cls_again, + "ClientServer mode: repeated walks of the same " + "FQN must hit the JVMView _attr_cache and return " + "the same JavaClass instance.") + + # Multi-thread walk: all threads share the JVMView + # but each gets its own pinned connection. The cache + # itself is shared across threads. + results = [None] * 8 + errors = [] + + def worker(idx): + try: + results[idx] = ( + client_server.jvm.java.lang.String) + except Exception as e: + errors.append(e) + + threads = [ + threading.Thread(target=worker, args=(i,)) + for i in range(8) + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + self.assertEqual(errors, []) + # All threads see the now-warm cache; identity + # stable. + for r in results: + self.assertIs(r, cls_first) + finally: + client_server.shutdown() + class RetryTest(unittest.TestCase): diff --git a/py4j-python/src/py4j/tests/java_gateway_test.py b/py4j-python/src/py4j/tests/java_gateway_test.py index 03578cbe..ed268399 100644 --- a/py4j-python/src/py4j/tests/java_gateway_test.py +++ b/py4j-python/src/py4j/tests/java_gateway_test.py @@ -670,6 +670,231 @@ def testDetach(self): len(ThreadSafeFinalizer.finalizers) - finalizers_size_start, 0) self.gateway.shutdown() + def testAttrCacheDoesNotAffectJavaObjectDetach(self): + # Regression test for the attribute-resolution cache on + # JVMView / JavaPackage / JavaClass. The cache memoizes + # JavaClass / JavaPackage / JavaMember (none of which carry + # JVM-side finalizable resources) but must never affect the + # JavaObject lifecycle, which is the GC machinery PySpark + # ML's JavaWrapper.__del__ depends on (SPARK-18274 et al.). + # + # Procedure: + # 1. Walk gateway.jvm.java.lang.StringBuffer (populates + # JVMView._attr_cache and the JavaPackage._attr_cache + # down the chain). + # 2. Construct a StringBuffer twice; check that + # ThreadSafeFinalizer registers two finalizers for the + # two JavaObjects. + # 3. detach() one, _detach() the other, gc.collect(). + # 4. Assert the finalizer-registry delta is zero — proof + # that the cache did NOT pin the JavaObject bindings. + self.gateway = JavaGateway() + gc.collect() + finalizers_size_start = len(ThreadSafeFinalizer.finalizers) + + # Populate the attribute cache on every level of the walk. + # The same walk repeated should hit cache for every segment. + cls_first = self.gateway.jvm.java.lang.StringBuffer + cls_second = self.gateway.jvm.java.lang.StringBuffer + # Cache identity: after the first walk, repeats return the + # *same* JavaClass instance. + self.assertIs(cls_first, cls_second) + + # Now construct two JavaObject instances and verify they are + # detached cleanly. The JavaObjects themselves must NOT be + # in any cache. + sb1 = cls_first() + sb1.append("Cache hit walk #1") + self.gateway.detach(sb1) + sb2 = cls_first() + sb2.append("Cache hit walk #2") + sb2._detach() + gc.collect() + + # If the attribute cache somehow retained a strong reference + # to the JavaObjects (it should not), the finalizer registry + # would still hold their entries here. + self.assertEqual( + len(ThreadSafeFinalizer.finalizers) - finalizers_size_start, + 0, + "Attribute cache must not pin JavaObject bindings — " + "ThreadSafeFinalizer registry leaked entries past " + "detach(). This breaks PySpark ML's JavaWrapper.__del__ " + "memory-management path.") + self.gateway.shutdown() + + def testConcurrentColdFqnWalk(self): + # 16 threads simultaneously walking the same cold FQN chain + # ``gateway.jvm.java.lang.StringBuffer``. The cache may race + # under contention — each thread might independently complete + # reflection and ``put``, so identity across threads is not + # guaranteed for the cold path. But no thread may raise; all + # must receive a valid ``JavaClass`` with the right ``_fqn``; + # the final cache state must be coherent. + import threading as _threading + self.gateway = JavaGateway() + n_threads = 16 + barrier = _threading.Barrier(n_threads, timeout=10) + results = [None] * n_threads + errors = [] + + def worker(idx): + try: + # Sync to t=0 so all threads race the cache populate. + barrier.wait() + results[idx] = self.gateway.jvm.java.lang.StringBuffer + except Exception as e: + errors.append(e) + + threads = [ + _threading.Thread(target=worker, args=(i,)) + for i in range(n_threads) + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + self.assertEqual(errors, []) + self.assertEqual(len(results), n_threads) + for r in results: + self.assertIsNotNone(r) + self.assertEqual(r._fqn, "java.lang.StringBuffer") + self.gateway.shutdown() + + def testAttrCacheSurvivesShutdownMidWalk(self): + # After ``JavaGateway.shutdown()``, the JVMView ``_attr_cache`` + # is cleared (Phase 2 of the cache PR). Subsequent attribute + # walks must surface a clean ``Py4JNetworkError`` from the + # broken socket connection — never an ``AttributeError``, + # ``KeyError``, or other internal-state exception leaking + # from a half-cleared cache. + self.gateway = JavaGateway() + # Populate the cache with a successful walk. + _ = self.gateway.jvm.java.lang.System + _ = self.gateway.jvm.java.lang.Integer + self.assertTrue(len(self.gateway.jvm._attr_cache._cache) > 0) + + # Shut down. Phase 2 clears jvm._attr_cache here. + self.gateway.shutdown() + self.assertEqual(len(self.gateway.jvm._attr_cache._cache), 0) + + # New walks must fail with the expected network error class, + # not a Python-internal exception. Py4JNetworkError is the + # documented contract; ConnectionRefusedError covers the + # case where the JVM listener also closed (this test class's + # tearDown will safe_join() the JVM either way). + with self.assertRaises((Py4JNetworkError, Py4JError)): + _ = self.gateway.jvm.java.lang.Long + + def testTwoGatewaysDoNotShareAttrCache(self): + # Each ``JavaGateway`` instance must have its own per-instance + # ``_attr_cache`` on its ``JVMView`` — no process-global cache, + # no cross-gateway leakage of cached ``JavaClass`` references. + gw1 = JavaGateway() + gw2 = JavaGateway() + try: + cls1 = gw1.jvm.java.lang.String + cls2 = gw2.jvm.java.lang.String + # Each gateway resolved the class independently — same + # FQN, different instances backed by different gateway + # clients. + self.assertIsNot( + cls1, cls2, + "Each JavaGateway must have its own JVMView " + "_attr_cache; JavaClass instances must not be " + "shared across gateways.") + self.assertIsNot( + gw1.jvm._attr_cache, gw2.jvm._attr_cache, + "JVMView _attr_cache must be per-instance, not " + "process-global.") + self.assertIs(cls1._gateway_client, gw1._gateway_client) + self.assertIs(cls2._gateway_client, gw2._gateway_client) + finally: + gw1.shutdown() + gw2.shutdown() + + def testMissesAreNotCached(self): + # The PR documents that only successful lookups are cached; + # misses (Py4JError) keep raising so later ``java_import`` or + # classpath changes can still surface a previously-missing + # class. + # + # Note on scope: only ``JavaClass.__getattr__`` produces real + # misses for unknown attribute names. ``JVMView`` and + # ``JavaPackage`` always treat unresolved names as new + # packages (the JVM-side ``REFL_GET_UNKNOWN`` returns + # ``SUCCESS_PACKAGE`` for any non-empty name not matching a + # class) — so misses on those paths are not user-observable + # as ``Py4JError``. This test pins the contract where it + # actually matters: ``JavaClass._members``. + self.gateway = JavaGateway() + system = self.gateway.jvm.java.lang.System + members_size_before = len(system._members._cache) + with self.assertRaises(Py4JError): + _ = system.noSuchMember + self.assertEqual( + len(system._members._cache), members_size_before, + "JavaClass._members must not store misses — keeping the " + "cache miss-free is what lets later ``java_import`` / " + "classpath changes still surface previously-missing names.") + self.gateway.shutdown() + + def testShutdownRaiseExceptionStillClearsCache(self): + # ``shutdown(raise_exception=True)`` re-raises any exception + # from the underlying gateway client. The defensive cache + # clear must still run via ``finally`` — otherwise an + # exception-path shutdown leaks cache entries that pin the + # gateway client. + self.gateway = JavaGateway() + _ = self.gateway.jvm.java.lang.System + self.assertGreater(len(self.gateway.jvm._attr_cache._cache), 0) + + # Force shutdown_gateway to raise by closing the underlying + # connection's socket first; the second shutdown will then + # raise on the dead socket. + try: + self.gateway._gateway_client.shutdown_gateway() + except Exception: + pass + # Now invoke shutdown with raise_exception=True. We don't + # care whether it raises or not for this test — only that + # the cache gets cleared by the finally clause. + try: + self.gateway.shutdown(raise_exception=True) + except Exception: + pass + + self.assertEqual( + len(self.gateway.jvm._attr_cache._cache), 0, + "shutdown(raise_exception=True) must still clear the " + "attribute cache via the finally clause, even when the " + "underlying gateway-client shutdown raises.") + + def testNoCycleInCachedInstances(self): + # After populating the cache and calling shutdown(), the + # JavaGateway must remain reclaimable by Python's refcount + # garbage collector. The cache holds JavaClass instances + # which hold _gateway_client — but neither of those reference + # JavaGateway back, so no cycle exists. + import weakref as _weakref + gateway = JavaGateway() + # Populate the cache deeply. + _ = gateway.jvm.java.lang.System + _ = gateway.jvm.java.lang.Integer.parseInt + wr = _weakref.ref(gateway) + gateway.shutdown() + del gateway + # gc.collect() is needed in case Python's tracing GC (not just + # refcount) is involved, e.g. if any future change introduces + # a soft cycle. + gc.collect() + self.assertIsNone( + wr(), + "JavaGateway should be reclaimable after shutdown() + " + "del; the attribute cache must not form a cycle that " + "pins the gateway in memory.") + def testGCCollect(self): self.gateway = JavaGateway() gc.collect() diff --git a/py4j-web/changelog.rst b/py4j-web/changelog.rst index c5d0e43e..1f9e39f2 100644 --- a/py4j-web/changelog.rst +++ b/py4j-web/changelog.rst @@ -7,6 +7,20 @@ releases. Unreleased ---------- +- Python side: Memoize attribute lookups on ``JVMView``, + ``JavaPackage``, and ``JavaClass`` via a per-instance bounded LRU + cache (default 1024 entries). Repeated walks along chains like + ``gateway.jvm.java.lang.System.currentTimeMillis()`` no longer + round-trip to the JVM for each segment after the first walk. + Identity semantics shift: ``gateway.jvm.X is gateway.jvm.X`` is + now ``True`` within the cache window (was ``False``). The cache + stores only non-finalizable types (``JavaClass``, + ``JavaPackage``, static-member ``JavaMember``); ``JavaObject`` + instances and their JVM-side finalization paths are unaffected. + ``JavaGateway.shutdown()`` drops every tracked ``JVMView``'s + attribute cache so cached references no longer pin the gateway + client past shutdown. Closes issue #128; refs #557. + - Java side: Fix listener-lifecycle correctness bugs in ``GatewayServer`` / ``ClientServer``: From 6a06668e76e8c01554a0ae62886b2ecd64339d8e Mon Sep 17 00:00:00 2001 From: Ruslan Dautkhanov Date: Tue, 9 Jun 2026 20:43:15 -0600 Subject: [PATCH 3/4] fix: revert shutdown() log levels to avoid pinning JavaGateway via pytest caplog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression introduced by the previous follow-up commit on this PR (elevation of ``logger.info`` to ``logger.warning`` in ``JavaGateway.shutdown()``): ``memory_leak_test::ClientServerTest`` started failing with ``AssertionError: 5 != 1`` on the matrix CI cells — finalization-counting tests saw only 1 of 5 expected JavaGateway-tree objects reach ``__del__``. Mechanism (verified end-to-end): 1. ``InstrClientServer`` constructs ``InstrJavaClient`` without passing ``finalizer_deque`` (default ``None``) — see ``py4j-python/src/py4j/tests/instrumented.py:148``. 2. ``clientserver.shutdown()`` calls ``JavaClient.shutdown_gateway()`` which has ``self.finalizer_deque.appendleft(SHUTDOWN_FINALIZER_WORKER)`` in a ``finally`` block — raises ``AttributeError: 'NoneType' object has no attribute 'appendleft'`` (``clientserver.py:253``). 3. ``JavaGateway.shutdown()`` outer ``except Exception`` catches it; with ``raise_exception=False`` (the test's default) it logs at WARNING with ``exc_info=True``. 4. pytest's ``caplog`` fixture captures records at WARNING level and above by default. The captured ``LogRecord`` retains the full ``exc_info = (exc_type, exc_value, traceback)``. 5. The ``traceback``'s ``tb_frame`` chain holds the current frame's locals — including ``self``, which is the ``JavaGateway`` / ``ClientServer`` instance and therefore the root of the entire object graph the test is trying to finalize. 6. As long as the test holds the captured ``LogRecord`` (i.e. until the test ends), the ``ClientServer`` tree (``_gateway_client``, ``_callback_server``, ``gateway_property``, ``ClientServerConnection``, ``PythonServer``, etc.) is reachable and cannot be GC'd. 7. The test's ``assertEqual(5, len(FINALIZED))`` sees only the ``InstrumentedObject`` finalize (the one object the test doesn't hold via locals). Below WARNING the record is discarded immediately by ``caplog``, so locals are released and the tree finalizes as expected. Fix: * ``shutdown()`` outer except: ``logger.warning`` -> ``logger.info`` (matches the pre-PR level — same diagnostic visibility for users running at INFO+, no caplog pin). * ``shutdown()`` cache-clear cleanup except: ``logger.warning`` -> ``logger.debug``. Same reasoning; debug is below caplog default. Not changed: * ``_BoundedAttrCache.get`` / ``put`` ``KeyError`` handlers KEEP ``logger.warning``. Those calls log only the ``name`` string (no ``exc_info=True``) — the captured ``LogRecord`` doesn't hold a frame containing ``self=JavaGateway``, so no graph pin. Baunsgaard's observability ask is still honored where it matters. Local verification: 7/7 ``ClientServerTest`` PASS with this fix on M-series + JDK 21 (was 6/7 fail with the prior commit). Co-authored-by: Isaac --- py4j-python/src/py4j/java_gateway.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/py4j-python/src/py4j/java_gateway.py b/py4j-python/src/py4j/java_gateway.py index 46aeca75..4a140960 100644 --- a/py4j-python/src/py4j/java_gateway.py +++ b/py4j-python/src/py4j/java_gateway.py @@ -2272,7 +2272,7 @@ def shutdown(self, raise_exception=False): if raise_exception: raise else: - logger.warning( + logger.info( "Exception while shutting down callback server", exc_info=True) self.shutdown_callback_server() @@ -2298,10 +2298,15 @@ def shutdown(self, raise_exception=False): except Exception: # Best-effort — never surface a cleanup exception # from shutdown; the gateway is already torn down at - # this point. Logged at warning so the failure is - # observable in diagnostics without breaking - # shutdown. - logger.warning( + # this point. Logged at debug (not warning) because + # ``logger.warning(..., exc_info=True)`` would have + # the captured traceback retain ``self`` (the + # JavaGateway) and its object graph through pytest's + # caplog — breaks memory-leak tests that count + # finalized objects. Debug level is below caplog's + # default capture, so the LogRecord is discarded + # and references are released. + logger.debug( "Exception while clearing JVMView attribute " "cache during shutdown", exc_info=True) From d00c042c4287fe35ddc3899c2ce7e5a33adf3bb2 Mon Sep 17 00:00:00 2001 From: Ruslan Dautkhanov Date: Wed, 10 Jun 2026 17:55:46 -0600 Subject: [PATCH 4/4] =?UTF-8?q?fix:=20address=20review=20feedback=20proper?= =?UTF-8?q?ly=20=E2=80=94=20root-cause=20+=20warning-without-pin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on this PR. (1) clientserver.py — fix latent bug in JavaClient.shutdown_gateway and JavaClient.garbage_collect_object that surfaced via the test failures CI flagged. InstrJavaClient (and any external subclass) can construct without propagating ``finalizer_deque``, leaving it at its default ``None``. Both ``shutdown_gateway`` (line 253) and ``garbage_collect_object`` (line 234) unconditionally call ``self.finalizer_deque.appendleft(...)`` — raising ``AttributeError: 'NoneType' object has no attribute 'appendleft'`` on the None deque. Guard both call sites: only enqueue if ``finalizer_deque is not None``; otherwise fall back to the synchronous super-class path (``garbage_collect_object``) or skip the SHUTDOWN sentinel (``shutdown_gateway``) because there is no worker thread to signal. (2) java_gateway.py — restore @Baunsgaard's logger.warning ask while avoiding the pytest-caplog test regression. The previous revert to logger.info was a symptom-silencer. The real fix: log at WARNING with the exception's class and ``str(e)`` as explicit message args, NOT ``exc_info=True``. Mechanism: - ``logger.warning(..., exc_info=True)`` produces a LogRecord that retains the exception's traceback. pytest's caplog captures records at WARNING+ by default; the captured traceback's ``tb_frame`` chain holds the current frame's locals — including ``self`` (the ``JavaGateway`` / ``ClientServer``) — for the test's lifetime. That pin prevents finalization-counting tests (``memory_leak_test::ClientServerTest``) from reaching their expected ``len(FINALIZED) == 5`` assertion. - Logging the exception's class name + message as string args keeps WARNING-level diagnostic visibility but produces a LogRecord whose msg arguments are plain strings — no frame references, no pin. Both shutdown sites updated: the outer ``except`` in ``JavaGateway.shutdown()`` and the cache-clear ``except`` in the same method's ``finally`` block. Result: Baunsgaard's WARNING level ask is now honored everywhere he flagged it, AND ``memory_leak_test::ClientServerTest`` stays green (the regression that motivated the prior logger.info revert is structurally fixed at the test-pin layer, not papered over). Co-authored-by: Isaac --- py4j-python/src/py4j/clientserver.py | 21 +++++++++-- py4j-python/src/py4j/java_gateway.py | 52 +++++++++++++++++++--------- 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/py4j-python/src/py4j/clientserver.py b/py4j-python/src/py4j/clientserver.py index 12c0d7da..8be2a25d 100644 --- a/py4j-python/src/py4j/clientserver.py +++ b/py4j-python/src/py4j/clientserver.py @@ -229,8 +229,15 @@ def garbage_collect_object(self, target_id, enqueue=True): JavaObject on the Python side. If enqueue is True, sends the request to the FinalizerWorker deque. Otherwise, sends the request to the Java side. + + When this ``JavaClient`` was constructed without a + ``finalizer_deque`` (which can happen on subclasses or in test + harnesses — see ``InstrJavaClient`` in ``tests/instrumented.py``), + there is no worker thread to drain the deque, so we fall back to + the synchronous super-class implementation regardless of + ``enqueue``. """ - if enqueue: + if enqueue and self.finalizer_deque is not None: self.finalizer_deque.appendleft((self, target_id)) else: super().garbage_collect_object(target_id) @@ -250,7 +257,17 @@ def shutdown_gateway(self): try: super().shutdown_gateway() finally: - self.finalizer_deque.appendleft(SHUTDOWN_FINALIZER_WORKER) + # The SHUTDOWN sentinel only makes sense if there's a + # ``FinalizerWorker`` consuming from the deque (i.e. this + # ``JavaClient`` was wired by a full ``ClientServer``). When + # constructed without a ``finalizer_deque`` — e.g. by an + # ``InstrJavaClient`` test subclass that does not propagate + # the kwarg — there is no worker to signal and the + # post-shutdown ``appendleft`` would raise ``AttributeError`` + # on the ``None`` deque. Guard the post-condition so the + # ``finally`` is safe regardless of construction path. + if self.finalizer_deque is not None: + self.finalizer_deque.appendleft(SHUTDOWN_FINALIZER_WORKER) def get_thread_connection(self): """Returns the ClientServerConnection associated with this thread. Can diff --git a/py4j-python/src/py4j/java_gateway.py b/py4j-python/src/py4j/java_gateway.py index 4a140960..72f0eac2 100644 --- a/py4j-python/src/py4j/java_gateway.py +++ b/py4j-python/src/py4j/java_gateway.py @@ -2268,13 +2268,35 @@ def shutdown(self, raise_exception=False): try: try: self._gateway_client.shutdown_gateway() - except Exception: + except Exception as e: if raise_exception: raise else: - logger.info( - "Exception while shutting down callback server", - exc_info=True) + # Log at WARNING for diagnostic visibility, but pin + # NO references through the LogRecord. Two pitfalls: + # + # 1. ``logger.warning(..., exc_info=True)`` retains + # the traceback explicitly via ``LogRecord.exc_info``. + # 2. Passing the exception INSTANCE ``e`` as a format + # arg ALSO retains the traceback — Python 3 + # exceptions carry ``__traceback__`` as an attribute, + # so ``LogRecord.args = (..., e)`` indirectly holds + # the frame chain. + # + # Either pitfall lets pytest's ``caplog`` (which + # captures records at WARNING level by default) pin + # ``self`` (``JavaGateway``) and its entire object + # graph for the test's lifetime — breaking + # finalization-counting tests + # (``memory_leak_test::ClientServerTest``). + # + # The pre-formatted f-string defuses both: only a + # plain string is passed to logger, and no args are + # retained. + logger.warning( + "Exception while shutting down callback " + "server: %s: %s", + type(e).__name__, str(e)) self.shutdown_callback_server() finally: # Defensive: drop the attribute-resolution cache on the @@ -2295,21 +2317,19 @@ def shutdown(self, raise_exception=False): attr_cache = getattr(jvm, "_attr_cache", None) if attr_cache is not None: attr_cache._cache.clear() - except Exception: + except Exception as e: # Best-effort — never surface a cleanup exception # from shutdown; the gateway is already torn down at - # this point. Logged at debug (not warning) because - # ``logger.warning(..., exc_info=True)`` would have - # the captured traceback retain ``self`` (the - # JavaGateway) and its object graph through pytest's - # caplog — breaks memory-leak tests that count - # finalized objects. Debug level is below caplog's - # default capture, so the LogRecord is discarded - # and references are released. - logger.debug( + # this point. Log at warning for diagnostic visibility + # but pre-evaluate ``str(e)`` (and avoid + # ``exc_info=True``) so the captured LogRecord does + # not retain the exception's ``__traceback__`` and the + # frame chain pinning ``self`` — see the rationale on + # the outer except above. + logger.warning( "Exception while clearing JVMView attribute " - "cache during shutdown", - exc_info=True) + "cache during shutdown: %s: %s", + type(e).__name__, str(e)) def shutdown_callback_server(self, raise_exception=False): """Shuts down the