PYTHON-5898 Reduce pool checkout lock overhead and stop mutating TopologyDescription during selection - #16
Closed
blink1073 wants to merge 25 commits into
Closed
PYTHON-5898 Reduce pool checkout lock overhead and stop mutating TopologyDescription during selection#16blink1073 wants to merge 25 commits into
blink1073 wants to merge 25 commits into
Conversation
_filter_servers() cached its per-call filtered list on self._candidate_servers, and apply_selector() read it back through the candidate_servers property. TopologyDescription is a shared, publicly exposed *immutable* snapshot, so this left candidate_servers permanently stale after any selection that deprioritized servers: the cached value from the last call, not the true set of known servers. This is a regression from PYTHON-5662 (4.16.0, commit 0cfba49), which changed Selection.from_topology_description() to default to candidate_servers instead of known_servers. On current main the stale cache makes Topology.get_primary() (and client.primary) raise IndexError on the unguarded selection[0] after any retryable operation deprioritized the primary, and makes client.secondaries/client.arbiters silently return stale results. All of these are single-threaded, user-facing symptoms, not a concurrency issue. Return the candidate list instead and pass it explicitly to Selection.from_topology_description(), restoring TopologyDescription to a genuinely immutable snapshot after construction and restoring candidate_servers to its pre-4.16.0 value.
Pool._get_conn() used to take self.lock, then self.size_cond, then self.lock again just to bump operation_count and requests/active_sockets on an uncontended checkout. self.lock, size_cond, and _max_connecting_cond all wrap the same underlying mutex, so those three acquisitions were serializing on one lock for no reason. On the fast (uncontended) path, do the operation_count, requests, and active_sockets bookkeeping in a single critical section under self.lock, falling through to the old size_cond wait loop only when no pool slot is immediately available. The contended (slow) path is otherwise unchanged. Also wrap the whole checkout body in try/except so operation_count is always decremented on failure, fixing a pre-existing leak where a failed checkout left operation_count incremented forever, skewing server selection among mongoses toward the affected server.
…failure Adds test_wait_queue_timeout_does_not_leak_operation_count to test/test_pooling.py and test/asynchronous/test_pooling.py, covering the "Merge pool checkout lock acquisitions on the fast path" fix: a checkout that fails while waiting for a free pool slot (wait queue timeout) must not leave Pool.operation_count permanently incremented for the failed attempt. Each test was verified to fail against the pre-fix code (operation_count left incremented instead of returning to its prior value), and passes after the fix.
The lock-merge refactor moved the pool-not-ready and wait-queue-timeout checks inside the outer try/except (needed for the operation_count leak fix), but didn't account for the outer except also running for those exceptions now. That caused a second, incorrect ConnectionCheckOutFailed event (reason connectionError) to follow the correct one whenever a checkout failed because the pool was paused/closed or because the wait queue timed out. Fix: the three _raise_if_not_ready call sites in the fast/slow path now pass emit_event=False and let the outer except emit once, and the wait-queue-timeout branch now sets emitted_event=True before raising, mirroring the existing max_connecting_cond pattern. Also removed the redundant slot_acquired flag (identical to requests_incremented at that point) and folded operation_count's decrement into the size_cond critical section in the except block, gating notify() on whether a slot was actually released. Extends the operation_count leak regression test to also assert on requests/active_sockets and on the emitted event count/reason, and adds a second regression test covering the pool-paused checkout-failure path, both of which fail against the pre-fix code with the double emission this commit fixes.
… test The Topology reader/writer lock explored on this branch was benchmarked ~30% slower and fully reverted, but three places still described apply_selector()/_filter_servers() as running under a "shared read lock" that no longer exists. Rewrite those docstrings/comments around the actual, still-true justification: TopologyDescription is a shared, immutable snapshot, so caching a per-call filtered list on it leaves the public candidate_servers property stale and can make Topology.get_primary() raise IndexError. Also renames TestTopologyDescriptionConcurrency to TestTopologyDescriptionImmutability to match, adds a regression test for the concrete get_primary()/IndexError symptom, adds a timeout to the existing racy test's barrier.wait() so a dead worker can't hang CI, and documents all three fixes in the changelog.
PYTHON-5662 added candidate_servers to plumb its deprioritization-filtered list into Selection.from_topology_description(), which took only a TopologyDescription. The list is now passed explicitly, so the property has no remaining consumer. Its documented contract ("servers excluding deprioritized servers") could not be satisfied anyway: deprioritization is an argument to a single selection call, so there is no single correct answer for a description that serves many calls. Selection now defaults to known_servers.
test_apply_selector_does_not_mutate_description asserted on the candidate_servers property; once that property was removed the assertion reduced to a tautology and passed against the unfixed code, so it was carrying no weight. The two remaining tests in the class both fail against the unfixed code and cover the same ground. Also drop the remaining references to the removed property, and stop describing TopologyDescription as immutable while explaining that it used to be mutated.
…w-path counters Restore upstream's ordering guarantee (PYTHON-3519) by having the three size_cond-region readiness checks publish ConnectionCheckOutFailed while holding the mutex, bracketed so the outer handler does not double-emit. Fold the slow path's active_sockets increment into the size_cond block, collapsing the two counter flags into one.
Register a CMAP listener that records pool.lock.locked() at the moment ConnectionCheckOutFailedEvent fires. Listeners run synchronously inside the publish call, so this observes directly whether the emitting code still holds the mutex, pinning the PYTHON-3519 ordering guarantee.
…ions candidate_servers returned a list of known servers; server_descriptions() returns a dict of every server including Unknown. known_servers is the exact analogue and is what selection now falls back to.
The old wording blamed concurrent selection calls, but select_servers(), get_primary() and _get_replica_set_members() all hold Topology._lock for the whole call, so those apply_selector() calls never overlap. The actual hazards are sequential state leakage into later readers of the same description, and genuinely concurrent reads from external callers such as topology event listeners and holders of client.topology_description.
Correct the class docstring's rationale to match topology_description. Surface unexpected worker exceptions (a BrokenBarrierError from the barrier timeout, say) through errors instead of letting threading print and discard them, which left errors empty and passed vacuously. Add a get_secondaries() regression test, since the stale-membership bug the changelog describes covered client.secondaries/arbiters too.
Set each flag immediately after its own increment, with no statement in between, matching upstream. A single combined flag left a window where an interrupt delivered between the two increments and the flag assignment would leak both counters.
…_servers removal as breaking
…path Nothing asserted the number of lock acquisitions, so splitting the merged counter bookkeeping back apart would have passed every other test in the file and silently undone the change this ticket is for.
Counterpart to the fast-path test. Folding the slot bookkeeping into the size_cond critical section removed an acquisition from the contended path, and nothing asserted it, so splitting it back apart would have gone unnoticed.
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
1 participant
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.
Changes in this PR
path. The contended path is unchanged.
the ordering guarantee from PYTHON-3519.
client.primaryraisingIndexError, andclient.secondariesandclient.arbitersreturning stale results, after a retryable operationdeprioritized the primary. Regression from PYTHON-5662.
TopologyDescription.candidate_servers, added in4.16.0. Its value depended on which selection call ran last.
known_serversis the direct replacement and is already public.
incremented a pool's
operation_count, so an affected mongos wasprogressively avoided by server selection.
Not pursued: the proposed Topology reader/writer lock
The ticket also proposed giving
Topologya reader/writer lock. That wasimplemented and then reverted: it made server selection about 30% slower at
concurrency 20 in every case measured, including topology churn.
Measuring the premise directly, under real operations rather than a tight
selection loop, the topology lock turns out not to be contended. Time spent
blocked waiting for it, as a share of aggregate worker time:
Contention falls as concurrency rises, and the mean wait per acquire stays at
roughly the cost of an uncontended acquire. Measured against localhost, where
the lock's share of total time is as large as it will ever be.
The ceiling on optimizing this lock is about 0.016% throughput, for any
approach. Suggest recording that on the ticket and dropping the work from its
scope.
Test Plan
Nine new tests. Each was run against the unfixed code and confirmed to fail
there, then against the fix and confirmed to pass:
client.primary,client.secondariesandclient.arbiterssymptomsoperation_countleak and checkout-failed event behaviour,on both the fast and the contended checkout path
reduction cannot be undone unnoticed
To confirm the selection change is otherwise behavior-preserving, selection
output was captured before and after across every server-selection and
max-staleness fixture, nine selectors, and every subset of deprioritized
servers. Results are identical apart from the
client.primarycalls thatpreviously raised.
Clean runs of the topology, server-selection, pool, connection-monitoring, and
SDAM suites, in both the async and sync flavours.
just synchroproduces nodrift,
just typingreports 0 errors,just lint-manualpasses.Checkout throughput at concurrency 20 is up 6.6%; server selection is unchanged.
Two pre-existing issues surfaced, neither introduced here:
process produces order-dependent
TestPoolMaxSizefailures on this branch andon unmodified main, a different set each time. All pass in isolation.
check_eventsonly fails on missing events and silentlytolerates extra ones, so duplicate events can pass unnoticed.
Checklist
Checklist for Author
PYTHON-5982 adds a concurrent checkout benchmark to the performance suite,
so a regression like the reader/writer lock's would be caught in CI.
Still to file: update PYTHON-5898 to drop the topology lock work from
scope; the two test issues above; deprioritization applies only to the
first attempt of a selection (from PYTHON-5662); and
tools/synchro.py's"aiter": "iter"replacement is not word-boundary aware, so it silentlycorrupts any identifier or comment containing "waiter".