Repository navigation
[SPIKE] sans-I/O extraction to reduce unasync duplication - #2918
Closed
aclark4life wants to merge 11 commits into
Closed
aclark4life wants to merge 11 commits into
aclark4life wants to merge 11 commits into
Conversation
Move pure protocol/decision logic out of the async/sync-duplicated layer into shared sans-I/O modules, as a proof of concept for reducing unasync's footprint. - network_layer.parse_wire_header: shared wire-message framing/validation used by both receive_message and PyMongoProtocol.process_header. Fixes the missing OP_COMPRESSED length<=25 guard on the sync path (PYTHON-5906): the sync path previously raised ValueError instead of ProtocolError. - pymongo/_sdam_error.py: pure SDAM application-error decision core (decide_error_action + _SDAMAction) plus the topologyVersion staleness helpers, testable without a live server. - topology._handle_error / _is_stale_error reduced to thin executors around the shared core. - test/test_sdam_error.py: server-free unit tests for the new core.
The predicate 'streaming enabled and server type + topologyVersion known' was duplicated at three sites in monitor.py (the streaming-continuation gate and two 'awaited' computations). Extract it into a shared sans-I/O helper is_streaming_check() in pymongo/_sdam_monitor.py, depending on a minimal structural Protocol rather than the full ServerDescription. - Deduplicates the predicate within monitor.py and removes it from unasync's scope (shared module, single implementation). - test/test_sdam_monitor.py: server-free unit tests. Verified: mypy+pyright clean, ruff clean, test_discovery_and_monitoring (180 mocked SDAM spec tests) pass.
Move the SDAM 'power of two random choices' load-balancing tie-break out of Topology._select_server into a shared, generic select_least_loaded() in pymongo/_sdam_selection.py. It is generic over the server type via a 'load' callable, so it does not depend on the flavor-specific Server class, and takes an injectable sampler for deterministic testing. - Removes the decision from unasync's scope; drops the now-unused 'random' import from topology.py. - test/test_sdam_selection.py: server-free unit tests.
Prove that a whole generated module can leave unasync. TopologySettings is pure configuration, but it was duplicated only because it defaulted pool_class/monitor_class to the flavor-specific pool.Pool/monitor.Monitor. - Move settings to a single shared pymongo/settings.py; delete the asynchronous/ and synchronous/ copies. - Relocate the flavor default from settings into topology.py (the layer that legitimately knows its flavor): 'self._settings.pool_class or Pool' etc. settings now stores pool_class/monitor_class as given (Optional). - Update all importers to 'from pymongo.settings import TopologySettings'. Because the default moved rather than being removed, every existing caller (including tests that omit the classes and rely on the default) keeps working with no behavior change. Cost of removing one module from unasync: ~17 files touched (mostly mechanical import swaps + regenerated twins), no shared Pool/Monitor base needed thanks to the default relocation.
Topology._error_message was ~55 lines of pure formatting over a TopologyDescription, duplicated verbatim in both flavors. Move it into a shared format_selection_error() in pymongo/_sdam_selection.py; topology now passes (description, selector, replica_set_name, seed_addresses) and delegates. - Removes the duplicated logic from unasync's scope. - test/test_sdam_selection.py: 6 server-free tests covering the no-servers / discovering / shared-error / distinct-errors / unreachable-seeds branches. Verified: mypy+pyright clean, ruff clean, test_topology + test_server_selection (sync and async) pass.
Two more pure decisions that lived in the generated topology.py: - is_stale_server_description: move next to its sibling staleness helpers in pymongo/_sdam_error.py (topologyVersion comparison; pure). - data_bearing_servers: extract the 'which servers can be selected' decision into pymongo/_sdam_selection.py; Topology keeps a thin delegating wrapper. Both removed from unasync's scope. Added server-free tests for each (building ServerDescriptions from Hello docs). Verified: mypy+pyright clean, ruff clean, test_topology passes.
Clarified the mention of a live example in the divergence risk section.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Motivation
PyMongo maintains two parallel drivers:
pymongo/asynchronous/(hand-written) andpymongo/synchronous/(generated byunasync). This duplicates logic that isn't actually async/sync-specific, which is both a maintenance burden and a divergence risk.This spike separates shared decision logic (no
await, no locks, no I/O — "sans-I/O") from the async/sync-specific wrapper (theawaits, condition variables, background loops that genuinely differ per driver). Pure logic moves into shared top-level modules imported by both drivers; the async/sync-specific wrapper stays generated.What's here (7 commits)
network_layer.parse_wire_header()shared byreceive_message(sync) andPyMongoProtocol.process_header(async).pymongo/_sdam_error.py:decide_error_action()(the application-error → action decision tree) + staleness helpers.pymongo/_sdam_monitor.py(is_streaming_check),pymongo/_sdam_selection.py(select_least_loaded,format_selection_error,data_bearing_servers).settings.pyis now a single sharedpymongo/settings.py(theasynchronous/synchronouscopies are deleted). The only thing forcing the split was an async/sync-specific default; that default was relocated intotopology.py(self._settings.pool_class or Pool), so no caller changed behavior.See
SANS_IO_SPIKE.mdon this branch for the full findings write-up.What the spike found
unasync— but the cost is the import graph, not the code's purity. Movingsettings.pytouched ~17 files (mostly mechanical import swaps + regenerated twins), because everyfrom pymongo.{asynchronous,synchronous}.settings importcaller — many themselves generated — had to be rewritten.unasync— the irreducibly async/sync-specific wrapper (locks, condition-variable waits, background loops, socket I/O) stays generated. But essentially all the logic can move out of it.Testing
just typing(mypy + pyright) clean,ruff/synchroclean, full topology/SDAM/selection suites pass (sync + async twins).mongodb-latest, both drivers, all topologies): green.prefixtext-query feature gap, mongod-start infra flakiness, timing-sensitive backpressure/handshake integration tests (which passed under the identical diff in the targeted patch), and a Windows infra glitch. The server-free variant carrying all the new sans-I/O unit tests passed. No failure is attributable to this diff.