perf: replace RLock with Lock where re-entrant locking is not needed (~11ns saving, -14%)

[mykaul]

Summary

Convert 7 of 8 RLock instances to plain Lock. All verified to use only flat (non-recursive) acquisition patterns:

Lock	File	Hot path?
Connection.lock	connection.py	Yes — every message send/receive
Cluster._lock	cluster.py	No — connect/shutdown only
ControlConnection._lock	cluster.py	No — schema/topology refresh
ControlConnection._reconnection_lock	cluster.py	No — reconnection only
Metadata._hosts_lock	metadata.py	No — host add/remove
TokenMap._rebuild_lock	metadata.py	No — keyspace rebuild
Host.lock	pool.py	No — reconnection handler
cqlengine.Connection.lazy_connect_lock	cqlengine/connection.py	No — lazy connect

Session._lock is kept as RLock because run_add_or_renew_pool() uses manual release()/acquire() inside a with block, which requires re-entrant semantics.

Benchmark

Lock type	Per-cycle (with stmt)	Overhead
RLock	92.9 ns	baseline
Lock	81.1 ns	-14%

Tests

• 9 focused unit tests verifying lock types and operations (metadata add/update/remove, host reconnection handler)
• test_update_host_sequential_lock specifically validates that Metadata.update_host() works with plain Lock (sequential, not nested acquisition)
• Full unit test suite passes (654 passed)

[mykaul]

V2 Changes

Fixed: deadlock in Cluster.connect() failure path

Cluster.connect() acquires self._lock, and on control connection failure the original code called self.shutdown() inside the with self._lock: block. Since shutdown() also acquires self._lock, this would deadlock with a plain Lock (any network failure during initial connection would trigger it).

Fix: Restructured connect() to save the exception and call shutdown() after the with self._lock: block exits, avoiding the re-entrant acquisition.

Additional cleanup:

• Removed unused RLock import from cassandra/connection.py
• Added TestClusterConnectFailureNoDeadlock test that verifies connect() calls shutdown() and re-raises without deadlocking when using a plain Lock

Tests: 683 unit tests pass (660 core + 23 IO), 0 failures.

[mykaul]

Follow-up: Skip lock acquisition when no orphaned requests in process_msg

Commit: 2e5a6c6

What changed

In process_msg(), the orphaned request check acquires a lock on every response to check self.orphaned_request_ids. Since orphaned requests only exist after timeouts (a rare event), the set is almost always empty.

Now we check if self.orphaned_request_ids: (set truthiness) before acquiring the lock. Empty set → skip the lock entirely.

Thread safety

The unlocked truthiness check on a set is safe under the GIL (atomic read of internal size field). Worst case:

• False positive (set non-empty but stream_id not in it): enters lock block, re-checks, safe
• False negative (set just got an entry): orphaned response processed normally — acceptable

Benchmark results (Python 3.14, 2M iterations)

Scenario	Before	After	Change
Empty orphaned set (common)	80.6 ns	23.2 ns	-57.4 ns (3.47x)
Non-empty set (rare)	79.7 ns	87.8 ns	+8.1 ns overhead

Testing

• 617 unit tests passed (10.4s)
• No regressions

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6bf1e333-72f2-4d0e-a787-01924eed9d5f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[mykaul]

Rebased onto current master + full re-entrancy re-verification

Rebased perf/rlock-to-lock onto origin/master (was 66 commits behind, including the DRIVER-153 SCYLLA_USE_METADATA_ID work, the shard-aware SSL fix, and the ProtocolFeatures serialization changes). One trivial conflict in cassandra/pool.py (both sides touched the import block: master added import uuid, this branch dropped RLock from the threading import) — resolved by keeping both. No functional changes were needed.

Given how much ResponseFuture/Cluster/connection-handling code has changed on master recently, I re-did the re-entrancy analysis from scratch against the current state of every file this PR touches, not just the original diff. Findings below, per lock.

1. Connection.lock (cassandra/connection.py)

Acquisition sites on current master: defunct(), error_all_requests(), wait_for_responses() (busy-wait send loop), process_msg() (both the new orphan fast-path and the stream-id-return-on-KeyError/finally paths), remove_continuous_paging_session(), set_keyspace_async(); plus per-reactor close() in all six cassandra/io/*.py backends, and external call sites in pool.py (borrow_connection, return_connection) and cluster.py ResponseFuture (_on_timeout, _borrow_control_connection, _release_control_connection_request, _handle_control_connection_response).
Every one of these critical sections is flat: the response/user callback (cb(response) in process_msg, callback(self, None) in set_keyspace_async, cb(response) in _handle_control_connection_response, etc.) is always invoked after the with self.lock: block has exited, never inside it. defunct() calls self.close() outside its own lock block, and close() in every reactor implementation only holds the lock for the is_closed flag check/set, never around the callback-firing/socket-teardown code. No path re-enters self.lock while already holding it. Safe as plain Lock.

2. Cluster._lock (cassandra/cluster.py)

Only two acquisition sites: connect() and shutdown(). This PR's own diff already fixes the one real re-entrancy hazard here: the original code called self.shutdown() (which acquires self._lock) from inside connect()'s with self._lock: block on control-connection failure — that would deadlock with a plain Lock. The fix defers shutdown()/re-raise until after the lock is released (see the connect_exc pattern). Verified this still holds against current master: control_connection.connect() (called inside the lock) does not call back into Cluster.connect()/Cluster.shutdown() synchronously anywhere in its current implementation. Safe.

3. ControlConnection._lock / _reconnection_lock

_lock acquired in _set_new_connection() and shutdown(); _reconnection_lock in _reconnect(), _get_and_set_reconnection_handler(), and shutdown(). All flat. shutdown() acquires _reconnection_lock and _lock in two separate, sequential with blocks (never nested). Push-event handlers (_handle_topology_change/_handle_status_change/_handle_schema_change/_handle_client_routes_change) dispatch via scheduler.schedule_unique(...) — i.e. asynchronously, never synchronously from inside a locked section — so no callback re-entry risk. Safe.

4. Metadata._hosts_lock

Acquisition sites: add_or_return_host, remove_host, remove_host_by_host_id, update_host, get_host, get_host_by_host_id, all_hosts, all_hosts_items, plus Cluster.add_host() in cluster.py. update_host() calls add_or_return_host() (acquire+release) and then acquires the lock again for the endpoint-map update — sequential, not nested. Cluster.add_host() does the same pattern (checks under the lock, releases, then calls add_or_return_host separately). This is exactly what tests/unit/test_rlock_to_lock.py::test_update_host_sequential_lock exercises. Safe.

5. TokenMap._rebuild_lock

Single acquisition site (rebuild_keyspace). get_replicas() calls rebuild_keyspace() but never while holding the lock itself. No nesting anywhere. Safe.

6. Host.lock

Acquisition sites: Host.get_and_set_reconnection_handler(), and in cluster.py's on_up/_on_up_future_completed/on_down. All critical sections are short flag/state updates. Notably, in on_up(), future.add_done_callback(callback) (which could fire synchronously if the future is already done) is registered outside any with host.lock: block, and the callback itself (_on_up_future_completed) only touches a separate, freshly-created futures_lock, acquiring host.lock only in its own finally clause after that lock is already released. listener.on_up(host) (user callback) also runs outside any lock. Safe.

7. cqlengine.connection.Connection.lazy_connect_lock

Single acquisition site (handle_lazy_connect). It sets self.lazy_connect = False before calling self.setup() while holding the lock; setup() never calls handle_lazy_connect() back. Even in a hypothetical reentrant call, handle_lazy_connect() checks if not self.lazy_connect: return before touching the lock, so a nested call would short-circuit before ever touching lazy_connect_lock. Safe.

Not converted (unchanged, confirmed still correct to leave as-is)

Session._lock remains an RLock. Session.add_or_renew_pool()'s inner run_add_or_renew_pool() does a manual self._lock.release() / self._lock.acquire() inside a with self._lock: block while waiting on a keyspace-change event — confirmed this pattern is unchanged on current master. (Manual release-then-reacquire on the same thread doesn't strictly require RLock semantics, since it isn't a nested acquire — but leaving it as RLock here is the conservative, zero-risk choice and out of scope for this PR, so it's untouched.)

Testing

• Full tests/unit/ (730 passed, 88 skipped) run with pytest-timeout (--timeout=60 --timeout-method=thread) — no hangs, no timeouts, ~21s total.
• Targeted re-run of test_rlock_to_lock.py, test_connection.py, test_cluster.py, test_control_connection.py, test_host_connection_pool.py, test_metadata.py, test_response_future.py, and tests/unit/cqlengine/ (217 passed, 30 skipped) — clean.
• tests/unit/io/ (per-reactor connection tests) — 10 passed, 42 skipped (server-dependent tests skip without a live cluster), no failures.
• No lock was reverted back to RLock — every conversion in this PR checks out against current master.

Branch has been force-pushed (rebased, no new commits) to mykaul:perf/rlock-to-lock. Still a draft pending final sign-off.

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Improve runtime performance by replacing re-entrant locks (RLock) with standard locks (Lock) where recursion isn’t needed, and add a small hot-path optimization to reduce unnecessary lock acquisitions.

Changes:

• Replace multiple RLock instances with Lock across connection/cluster/metadata/pool/cqlengine.
• Adjust Cluster.connect() failure path to avoid deadlock with non-reentrant locking.
• Add unit tests and micro-benchmarks to validate lock types and measure overhead.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
cassandra/connection.py	Switch Connection.lock to Lock and add a fast-path to skip locking in process_msg when possible.
cassandra/cluster.py	Switch internal locks to Lock and restructure connect() failure handling to avoid deadlock.
cassandra/metadata.py	Switch metadata/token map locks from RLock to Lock.
cassandra/pool.py	Switch Host.lock from RLock to Lock.
cassandra/cqlengine/connection.py	Switch lazy_connect_lock from RLock to Lock.
tests/unit/test_rlock_to_lock.py	Add unit tests validating lock types and basic operations don’t deadlock.
benchmarks/micro/bench_rlock_vs_lock.py	Add micro-benchmark comparing Lock vs RLock overhead.
benchmarks/micro/bench_orphan_lock_skip.py	Add micro-benchmark for the new orphaned-request “skip lock if empty” approach.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (4)

cassandra/cluster.py:1842

• Releasing _lock before marking the cluster shut down breaks the previous atomic failure transition. A second thread can enter connect() in this gap, retry setup, and even create a session that the first thread immediately shuts down. Preserve the is_shutdown transition under this same lock, then perform cleanup outside it via a non-locking helper so concurrent callers cannot observe a connectable cluster after setup failure.

        if connect_exc is not None:
            # shutdown() acquires self._lock, so must be called after
            # releasing it above to avoid deadlock.
            self.shutdown()

benchmarks/micro/bench_orphan_lock_skip.py:48

• This benchmark measures the superseded unlocked truthiness pre-check, not the implementation now used by process_msg (which only checks the orphan set after _requests.pop() raises). Its reported “new” timing is therefore unrelated to the code being merged and benchmarks the race-prone approach removed earlier in this PR. Update it to model the current try/except path or remove it.

    # New: check set first, skip lock if empty
    def new_check():
        nonlocal in_flight
        if orphaned_set:
            with lock:

cassandra/pool.py:24

• uuid is not referenced anywhere in cassandra/pool.py; this newly added import is unrelated to the lock conversion and should be removed.

import uuid

cassandra/connection.py:1425

• This still misses the interleaving where _on_timeout pops _requests, pauses before acquiring connection.lock, and this handler acquires the lock first. The ID is then returned without orphan bookkeeping; afterward _on_timeout adds a stale orphan entry and leaves in_flight elevated, even though the response has already arrived. The timeout path must make removing the request and publishing its orphan state atomic under the same lock (and add a regression test for this ordering).

                    with self.lock:
                        if stream_id in self.orphaned_request_ids:
                            self.in_flight -= 1
                            self.orphaned_request_ids.remove(stream_id)
                            need_notify_of_release = True
                        self.request_ids.append(stream_id)