rtc: fail fast instead of hanging when FFI is used across fork()#740
rtc: fail fast instead of hanging when FFI is used across fork()#740theomonnom wants to merge 1 commit into
Conversation
theomonnom
requested review from
cloudwebrtc,
lukasIO and
xianshijing-lk
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| assert FfiClient.instance._ffi_lib.livekit_ffi_drop_handle(ctypes.c_uint64(self.handle)) | ||
| ffi = FfiClient._owned_instance() | ||
| if ffi is not None: | ||
| assert ffi._ffi_lib.livekit_ffi_drop_handle(ctypes.c_uint64(self.handle)) |
There was a problem hiding this comment.
will assert be stripped out if our customers use some optimization flags ?
if that gets stripped out, I will assume ffi._ffi_lib.livekit_ffi_drop_handle will never be called.
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm assuming you will address the comment about 'assert'
theomonnom
force-pushed
the
theo/ffi-fork-guard
branch
from
e192426 to
d06cfdd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
theomonnom
force-pushed
the
theo/ffi-fork-guard
branch
2 times, most recently
from
3a092f3 to
389b573
Compare
|
oh, one question, is there any way to add a test for the fork behavior ? so that we can prevent regression |
The native runtime (tokio worker/IO threads in liblivekit_ffi) does not survive fork() and cannot be rebuilt by re-initializing, so a child that inherited an initialized FfiClient would hang on every request. Guard request() with the owning pid and raise a clear error instead. The atexit dispose handler is likewise skipped in fork children (it needs the runtime and would hang at exit). Also move the drop_handle side effect out of an assert so it is not stripped under python -O.
theomonnom
force-pushed
the
theo/ffi-fork-guard
branch
from
389b573 to
5fdf66e
Compare
There was a problem hiding this comment.
🔍 Callback still uses auto-creating singleton property, inconsistent with dispose()
The ffi_event_callback at livekit-rtc/livekit/rtc/_ffi_client.py:194 still accesses FfiClient.instance (the auto-creating @classproperty), while FfiHandle.dispose() was deliberately changed to use FfiClient._instance (the raw class variable) to avoid auto-creation during shutdown or in forked children. In practice this is likely safe because native threads aren't inherited across fork(), so the callback shouldn't fire in a child process. However, for consistency with the fork-safety intent of this PR, it might be worth using FfiClient._instance here too (with a None guard).
(Refers to line 194)
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
The native runtime (tokio worker/IO threads in liblivekit_ffi) does not survive
fork(), and re-initializing only resets the event callback rather than rebuilding the runtime. A child process that inherited an already-initializedFfiClientsingleton therefore hangs on every FFI request.Guard
FfiClient.instancewith the owning pid and raise a clearRuntimeErrorinstead of hanging. This does not affectspawn/forkserver(each process initializes its own FFI); it only turns thefork-after-init footgun into an actionable error.