CSHARP-3556: Reconsider server pinning in GridFS#1958
CSHARP-3556: Reconsider server pinning in GridFS#1958sanych-sun wants to merge 1 commit intomongodb:mainfrom
Conversation
sanych-sun
requested review from
BorisDog,
ajcvickers,
Copilot and
papafe
and removed request for
ajcvickers
sanych-sun
added
the
improvement
There was a problem hiding this comment.
Pull request overview
This PR removes a busy-wait server-availability polling loop from SingleServerReadBinding and instead relies on cluster-level server selection pinned by endpoint, aligning with the CSHARP-3556 TODO and improving correctness/behavior under disconnections.
Changes:
- Reworked
SingleServerReadBindingto select a server viaIClusterInternal.SelectServer/SelectServerAsyncusing anEndPointServerSelector(removing SpinWait-based polling and timeout plumbing). - Added an
EndPointaccessor toEndPointServerSelectorto make selector intent observable (used by tests). - Updated GridFS bucket and tests to use the new binding constructor and validate server selection behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Core/Clusters/ServerSelectors/EndPointServerSelectorTests.cs | Refactors selector tests into theory-based cases with a richer cluster description. |
| tests/MongoDB.Driver.Tests/Core/Bindings/SingleServerReadBindingTests.cs | Updates tests for new binding constructor and verifies cluster-based endpoint selection. |
| src/MongoDB.Driver/GridFS/GridFSBucket.cs | Constructs SingleServerReadBinding with the cluster to enable cluster-level selection. |
| src/MongoDB.Driver/Core/Clusters/ServerSelectors/EndPointServerSelector.cs | Exposes selector endpoint via a public property to support verification/diagnostics. |
| src/MongoDB.Driver/Core/Bindings/SingleServerReadBinding.cs | Replaces SpinWait polling with cluster server selection pinned by endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _server = Ensure.IsNotNull(server, nameof(server)); | ||
| _cluster = Ensure.IsNotNull(cluster, nameof(cluster)); | ||
| Ensure.IsNotNull(server, nameof(server)); | ||
| _serverSelector = new EndPointServerSelector(server.EndPoint); |
There was a problem hiding this comment.
SingleServerReadBinding builds an EndPointServerSelector from server.EndPoint without validating that server.EndPoint is non-null. If an IServer implementation (or a mock) returns null for EndPoint, this will throw an ArgumentNullException for parameter endPoint from inside EndPointServerSelector, which is harder to diagnose from the SingleServerReadBinding API surface. Consider explicitly validating server.EndPoint (and throwing an ArgumentNullException with a clear paramName like "server.EndPoint") before constructing the selector.
| _serverSelector = new EndPointServerSelector(server.EndPoint); | |
| var endPoint = Ensure.IsNotNull(server.EndPoint, "server.EndPoint"); | |
| _serverSelector = new EndPointServerSelector(endPoint); |
| _ = await subject.GetReadChannelSourceAsync(OperationContext.NoTimeout); | ||
| clusterMock.Verify(c => c.SelectServerAsync(It.IsAny<OperationContext>(), It.Is<EndPointServerSelector>(s => s.EndPoint == endpoint))); | ||
| } | ||
|
|
||
| private SingleServerReadBinding CreateSubject(IServer server = null, ReadPreference readPreference = null, ICoreSessionHandle session = null) | ||
| else | ||
| { | ||
| return new SingleServerReadBinding( | ||
| server ?? CreateMockServer().Object, | ||
| readPreference ?? ReadPreference.Primary, | ||
| session ?? new Mock<ICoreSessionHandle>().Object, | ||
| __defaultServerSelectionTimeout); | ||
| _ = subject.GetReadChannelSource(OperationContext.NoTimeout); | ||
| clusterMock.Verify(c => c.SelectServer(It.IsAny<OperationContext>(), It.Is<EndPointServerSelector>(s => s.EndPoint == endpoint))); |
There was a problem hiding this comment.
The Moq verification uses s.EndPoint == endpoint, which is reference equality for EndPoint implementations like DnsEndPoint. This makes the test brittle if EndPoint values are equal but not the same instance (e.g., if the selector is constructed from a different but equivalent EndPoint). Use value equality instead (e.g., EndPointHelper.Equals(s.EndPoint, endpoint) or Equals(s.EndPoint, endpoint)) to match the selector's comparison logic.
The PR replaces a busy-wait SpinWait.SpinUntil polling loop with proper cluster-level server selection, as the old TODO CSHARP-3556 comment indicated was needed.