fix(ffi): make unified set_callback one-shot while streaming is live#864
Merged
Conversation
Callback registration on the unified client is one-shot for the duration of a live session and serialised against concurrent self-calls, matching the FPSS path and the documented -1 contract. The entry point previously overwrote the stored callback and re-entered start_streaming unconditionally, so a second call while a session was live could leave the started session bound to a callback that diverges from the one stored on the handle, and a racing self-call could install one callback while another started a different one. The install now holds handle.callback across the gate, the store, and start_streaming, the same way the FPSS path holds dispatcher across its gate and open_fpss. A live session is detected before any state mutation and rejected with -1 and "streaming already started", leaving the live registration untouched. Replacement after thetadatadx_client_stop_streaming or _reconnect is still permitted because the slot is no longer live at that point. The dispatcher invokes the callback captured by copy, never reading the mutex, so holding the lock across start_streaming cannot deadlock the first delivered event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Callback registration on the unified client is one-shot for the duration of a live session and serialised against concurrent self-calls, matching the FPSS path and the documented -1 contract.
Defect
thetadatadx_client_set_callbackdocumented (in its Rust docstring and the C header) that a second call while streaming is already active returns -1 with"streaming already started", but the implementation had no such gate. It unconditionally overwrote the stored(callback, ctx)and re-enteredstart_streaming. Two consequences, same root:ctx, returning 0 with no drain barrier — the documented-1contract was never enforced.callbackmutex was taken to store, released, thenstart_streamingwas called separately, so two racing threads could store different callbacks and the live consumer'sctxcould diverge from the stored one.Fix
Bring the unified entry point to the same discipline the FPSS sibling
thetadatadx_streaming_set_callbackalready uses (gate held under one lock across the install). The unified handle's serialisation point is itscallbackmutex, so the install now holdshandle.callbackacross the gate, the store, andstart_streaming— the same shape as the FPSS path holdingdispatcheracrossreject_if_not_fresh+open_fpss. A live session is detected viais_streaming()before any state mutation and rejected with -1 and"streaming already started", leaving the live registration untouched.Replacement after
thetadatadx_client_stop_streaming/_reconnectis still permitted because the slot is no longer live at that point — the unified path keeps its documented high-level stop-then-re-register flow. The dispatcher invokes the callback captured by copy into the closure, never reading the mutex, so holding the lock acrossstart_streamingcannot deadlock the first delivered event. The documented contract (Rust docstring + C header) already matched; this makes the implementation match the documentation rather than the other way round.Test
Extended the unified set_callback guard tests to drive the real C ABI entry point: a null handle returns -1 with
"unified handle is null", a null callback returns -1 with"callback function pointer is null"(rejected before any handle dereference), and the live-handle gate's"streaming already started"contract string is pinned so the documented C ABI wording cannot drift from the implementation. Mirrors the FPSS null/double-callback guard tests.Verification
cargo check -p thetadatadx-ffi— cleancargo clippy -p thetadatadx-ffi --all-targets -- -D warnings— cleancargo test -p thetadatadx-ffi— all pass (4 new unit tests green)cargo fmt --all -- --check— cleanpython3 scripts/check_binding_parity.py— clean🤖 Generated with Claude Code