fix: drain in-flight commands before close frees the handle - #283
Draft
Aryex wants to merge 1 commit into
Draft
Conversation
#2) Close-vs-close was already serialized via `@close_lock`, but close-vs-in-flight-command was not. A thread inside `Bindings.command` / `Bindings.batch` / `Bindings.invoke_script` (all `blocking: true`, so they release the GVL and run in native code) could still hold the raw client handle when another thread's `close` freed it via `Bindings.close_client`, tearing down the connection under a live dispatch. Observed in CI as a `malloc(): unaligned tcache chunk detected` SIGABRT. Add a drain protocol guarded by `@close_lock`: - `@inflight` counter + `@drain_cv` ConditionVariable, initialized alongside `@close_lock` in `connection!`. - `send_command`, `send_batch_commands`, and `invoke_script` bracket their FFI dispatch with `acquire_connection_slot` / `release_connection_slot`. The lock is held only around the counter bump/decrement, never across the FFI call, so concurrent in-flight commands are not serialized. - `close` is two-phase: phase 1 sets `@closing` under `try_lock` (trap-safe), phase 2 waits on `@drain_cv` until `@inflight` reaches zero, then nulls the handle and calls `close_client`. Degrades to a lock-free best-effort close on `ThreadError` so `Signal.trap` shutdown still works. `acquire_connection_slot` refuses new slots once `@closing` is set, which is what makes the drain finite. `connection!` is retained for the fast fail-fast path. Add `test_close_drains_in_flight_commands_no_crash`, which races many worker threads issuing commands against a concurrent `close` and asserts the client ends up cleanly closed with a balanced in-flight counter. Signed-off-by: Alex Le <alex.le@improving.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.
Problem
Close-vs-close was already serialized via
@close_lock(#224), but close-vs-in-flight-command was not. A thread insideBindings.command/Bindings.batch/Bindings.invoke_script— allblocking: true, so they release the GVL and run in native code — could still hold the raw client handle when another thread'sclosefreed it viaBindings.close_client, tearing down the connection under a live dispatch.Observed in CI as a
malloc(): unaligned tcache chunk detectedSIGABRT. This is issue #212, race #2 (distinct from the close/close double-free already fixed).Fix
A drain protocol guarded by
@close_lock:@inflightcounter +@drain_cvConditionVariable, initialized alongside@close_lockinconnection!.send_command,send_batch_commands,invoke_scriptbracket their FFI dispatch withacquire_connection_slot/release_connection_slot. The lock is held only around the counter bump/decrement — never across the FFI call — so concurrent in-flight commands are not serialized (glide-core supports concurrent in-flight commands per client).closeis two-phase: phase 1 sets@closingundertry_lock(trap-safe); phase 2 waits on@drain_cvuntil@inflightreaches zero, then nulls the handle and callsclose_client. Degrades to a lock-free best-effort close onThreadErrorsoSignal.trap("TERM") { client.close }shutdown still works.acquire_connection_slotrefuses new slots once@closingis set — this is what makes the drain finite.connection!is retained for the fast fail-fast path.Testing
test_close_drains_in_flight_commands_no_crash: many worker threads race commands against a concurrentclose; asserts the client ends cleanly closed (the client is closed) with a balanced@inflightcounter.bundle exec rubocop— clean on all 3 files.test_close_racing_in_flight_commands_does_not_crash.Closes #284