CSHARP-5711: Avoid clearing the connection pool when the server connection rate limiter triggers #1838
Conversation
…ction rate limiter triggers
There was a problem hiding this comment.
Pull request overview
This PR implements support for avoiding connection pool clearing when the server connection rate limiter triggers, adding the "SystemOverloadedError" and "RetryableError" labels to network errors during connection establishment or handshake. This allows the driver to apply backpressure instead of unnecessarily clearing the pool.
- Added error label support to identify rate limiter backpressure scenarios
- Modified pool clearing logic to skip clearing when SystemOverloadedError is present
- Updated test specifications to reflect the new behavior
Reviewed changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MongoDB.Driver/Core/Connections/BinaryConnection.cs | Adds backpressure error labels to network errors during connection establishment/handshake |
| src/MongoDB.Driver/Core/Servers/DefaultServer.cs | Prevents pool clearing when SystemOverloadedError label is present |
| src/MongoDB.Driver/Core/Misc/Feature.cs | Adds IngressConnectionEstablishmentRateLimiter feature flag for server version 8.0+ |
| tests/MongoDB.Driver.Tests/Specifications/server-discovery-and-monitoring/prose-tests/ServerDiscoveryProseTests.cs | Adds prose test validating pool isn't cleared during backpressure |
| tests/MongoDB.Driver.Tests/Specifications/connection-monitoring-and-pooling/ConnectionMonitoringAndPoolingTestRunner.cs | Updates test runner to handle interruptInUseConnections parameter |
| tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs | Removes skip exception for previously flaky pool-clear tests |
| specifications/server-discovery-and-monitoring/tests/unified/.yml/.json | Updates schema versions from 1.10 to 1.4 and test descriptions |
| specifications/connection-monitoring-and-pooling/tests/cmap-format/.yml/.json | Renames closeInUseConnections to interruptInUseConnections throughout tests |
| specifications/load-balancers/tests/.yml/.json | Changes test expectations from network errors to server errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // https://github.com/mongodb/specifications/blob/a8d34be0df234365600a9269af5a463f581562fd/source/server-discovery-and-monitoring/server-discovery-and-monitoring-tests.md?plain=1#L176 | ||
| [Theory] | ||
| [ParameterAttributeData] | ||
| public async Task Connection_Pool_Backpressure([Values(true, false)]bool async) |
There was a problem hiding this comment.
Missing space between parameter attribute and parameter type. Add space after closing bracket of Values attribute.
| public async Task Connection_Pool_Backpressure([Values(true, false)]bool async) | |
| public async Task Connection_Pool_Backpressure([Values(true, false)] bool async) |
specifications/server-discovery-and-monitoring/tests/unified/minPoolSize-error.yml
Show resolved
Hide resolved
|
|
||
| _ = await ThreadingUtilities.ExecuteTasksOnNewThreadsCollectExceptions(100, async i => | ||
| { | ||
| await Task.Yield(); |
| public static Feature HintForUpdateAndReplaceOperations => __hintForUpdateAndReplaceOperations; | ||
|
|
||
| /// <summary> | ||
| /// Ingress Connection Establishment Rate Limiter feature |
There was a problem hiding this comment.
nit:
- period in the end.
Gets the .... feature.
| return null; | ||
| } | ||
|
|
||
| if (ex is MongoConnectionException mongoConnectionException) |
There was a problem hiding this comment.
minor: should this be a first statement?
There was a problem hiding this comment.
Nope, because MongoAuthenticationException inherits from MongoConnectionException, but we still want to return null for it, so we will re-throw the original exception and will not affect the stack trace.
| } | ||
|
|
||
| // private methods | ||
| private void AddBackpressureErrorLabelsIfRequired(MongoConnectionException exception) |
There was a problem hiding this comment.
I can see that WrapExceptionIfRequired can return null and thus the input exception here can be null as well. Should we add a null check?
papafe
left a comment
papafe
left a comment
There was a problem hiding this comment.
Just a small comment, but looks good otherwise.
3 participants
No description provided.