Repository navigation
Conversation
367bec6 to
5d02372
Compare
There was a problem hiding this comment.
📋 Review Summary
This PR prepares MultiProcessShared(..., spawn_process=True) for broader usage (issue #40673) by serializing constructors with apache_beam.internal.pickler, preventing unsafe_hard_delete() from spawning new processes on nonexistent tags, and ensuring custom __del__ finalizers run before os._exit(0). The implementation is clean and well-tested; however, the sys.getrefcount(obj) > 2 check in _SingletonEntry.unsafe_hard_delete is brittle on Python 3.14+ where LOAD_FAST_BORROW optimizes local variable references.
🔍 General Feedback
- Python 3.14 Compatibility (
sys.getrefcount): Starting in Python 3.14 (PEP 709 /LOAD_FAST_BORROWbytecode optimization), passing a local variableobjtosys.getrefcount(obj)uses a borrowed reference on the evaluation stack rather than incrementing the refcount. When no in-flight RPC holds a reference,sys.getrefcount(obj)returns1instead of2(or2instead of3when one_SingletonProxyor in-flight RPC reference exists). Callingtype(obj).__del__(obj)unconditionally (or movingdel objafter aweakref.refcheck) avoids relying on CPython bytecode refcount semantics.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| # still hold a stack reference to `obj`, preventing CPython from | ||
| # invoking `obj.__del__()` before `os._exit(0)`. | ||
| if (self._hard_delete_callback is not None and | ||
| sys.getrefcount(obj) > 2 and hasattr(type(obj), '__del__')): |
There was a problem hiding this comment.
🟡 Medium: sys.getrefcount(obj) > 2 relies on CPython incrementing the reference count both for the local variable obj and for passing obj as an argument to sys.getrefcount(). In Python 3.14+, the compiler emits LOAD_FAST_BORROW for local variable loads, which does not increment the reference count when pushing obj onto the stack for sys.getrefcount(obj). As a result:
- When only one in-flight thread holds a reference to
obj(total actual references = 2:objlocal + the in-flight frame),sys.getrefcount(obj)returns2on Python 3.14+, so> 2evaluates toFalseand__del__will be skipped beforeos._exit(0). - Conversely, if
del objfails to drop the refcount to zero (andos._exit(0)is about to terminate the process anyway whenself._hard_delete_callback is not None), we can avoidsys.getrefcountaltogether by checking aweakrefafterdel objor invokingtype(obj).__del__(obj)beforeself._hard_delete_callback()exits the process.
There was a problem hiding this comment.
Potentially check for other holders with a weakref instead (drop our reference, then see whether ref() still returns the object)?
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| if (self._hard_delete_callback is not None and | ||
| sys.getrefcount(obj) > 2 and hasattr(type(obj), '__del__')): | ||
| try: | ||
| type(obj).__del__(obj) |
There was a problem hiding this comment.
Could this hang for a model handler with a lock on the model + with_exception_handling(timeout ?
- The worker calls predict.
- It runs in the model process, takes the model's lock and hangs.
- The timeout fires in the worker.
- The worker stops waiting and schedules the model for deletion. Predict is still hung and still holding the lock.
- The worker loads a fresh copy of the model and carries on.
- At least 2× the timeout later, finish_bundle sends "hard delete" to the old model process. Forget the model, notice predict still holds it, and call the model's cleanup first. The cleanup waits for the lock, which predict never releases, so os._exit never runs and finish_bundle waits forever for a reply.
Potentially run the cleanup on a separate thread and wait for it only up to a timeout, then exit regardless, so a cleanup stuck behind the hung call can't block the hard delete?
| if (self._hard_delete_callback is not None and | ||
| sys.getrefcount(obj) > 2 and hasattr(type(obj), '__del__')): | ||
| try: | ||
| type(obj).__del__(obj) |
There was a problem hiding this comment.
The model's cleanup potentially runs twice with with_exception_handling(timeout) if the hung call finishes while the cleanup is still running?
- A call on the model hangs. The timeout fires, and the worker schedules the model for deletion. The call is still running in the model process.
- Later, the hard delete sees the call still holds the model (
sys.getrefcount(obj) > 2), so it callstype(obj).__del__(obj)(run 1). - The cleanup makes the hung call end, e.g. by killing a subprocess the call was polling. Once the call ends and its worker thread exits, nothing else holds the model.
- Run 1 returns and
del objdrops the last reference. Python frees the model and runs__del__again (run 2), because it doesn't know about run 1.
A cleanup that isn't safe to repeat would then run twice. Potentially keep the model referenced until exit after running the cleanup (e.g. append it to a module-level list), so __del__ can't run a second time?
| # still hold a stack reference to `obj`, preventing CPython from | ||
| # invoking `obj.__del__()` before `os._exit(0)`. | ||
| if (self._hard_delete_callback is not None and | ||
| sys.getrefcount(obj) > 2 and hasattr(type(obj), '__del__')): |
There was a problem hiding this comment.
Potentially check for other holders with a weakref instead (drop our reference, then see whether ref() still returns the object)?
Summary
Prerequisite
MultiProcessShared(..., spawn_process=True)improvements for #40673, split out ahead of theRunInferencechanges:apache_beam.internal.pickler:multiprocessing.get_context('spawn')uses standard-librarypickleforProcess(args=...), which rejects lambdas, closures, and local callables (AttributeError: Can't get local object ...). Serializingself._constructorwithapache_beam.internal.picklerallowsMultiProcessShared(lambda: ..., spawn_process=True)and objects holding local functions to spawn cleanly.MultiProcessShared.unsafe_hard_delete()safe for spawned processes:.addressfile, avoiding accidentally spawning a new server process just to delete it.(EOFError, OSError, BrokenPipeError)disconnects when the spawned server exits viaos._exit(0)insidehandle_unsafe_hard_delete.__del__finalizers beforeos._exit(0)during in-flight calls: If an in-flight RPC thread (such as a timed-out call) still holds a stack reference toself.objwhen_SingletonEntry.unsafe_hard_delete()runs in a spawned server process,del self.objalone does not drop the refcount to zero beforeos._exit(0)terminates the process. Explicitly invokingtype(obj).__del__(obj)ensures custom cleanup hooks still execute.Testing
MultiProcessSharedSpawnProcessTest(multi_process_shared_test.py) covering:Server.accepterdeadlock regression ([Bug]: Using MultiProcessShared handle can deadlock when always_proxy=True #40673) withalways_proxy=True, spawn_process=Truespawn_process=Trueunsafe_hard_delete()on non-existent tags (no-op, no extra process spawned)unsafe_hard_delete()running__del__while a blocked RPC is in flight