CSHARP-6080: Pool clear with closeInUseConnections leaks raw ObjectDisposedException to the application instead of a retryable connection error#2042
Conversation
sanych-sun
requested review from
BorisDog and
ajcvickers
and removed request for
ajcvickers
There was a problem hiding this comment.
Pull request overview
This PR addresses CSHARP-6080 by ensuring that when an ExclusiveConnectionPool is cleared with closeInUseConnections: true, in-flight operations observe a retryable MongoConnectionException instead of a raw ObjectDisposedException.
Changes:
- Added new tests validating
SendMessage/ReceiveMessagethrowMongoConnectionExceptionwhen interrupted by a pool clear that closes in-use connections. - Updated
ExclusiveConnectionPool’s acquired-connection wrapper to catchObjectDisposedExceptionfrom underlying connections and rethrow asMongoConnectionException.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Core/ConnectionPools/AcquiredConnectionTests.cs | Adds regression tests for pool-clear interruption behavior for send/receive. |
| src/MongoDB.Driver/Core/ConnectionPools/ExclusiveConnectionPool.Helpers.cs | Wraps ObjectDisposedException from acquired connections into MongoConnectionException for retryable error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sposedException to the application instead of a retryable connection error
| _connection.Open(operationContext); | ||
| SetEffectiveGenerationIfRequired(_connection.Description); | ||
| } | ||
| catch (ObjectDisposedException ex) when (_closedByPoolClear) |
There was a problem hiding this comment.
What about Reauthenticate? If it's not needed, add a comment why?
There was a problem hiding this comment.
There is no ThrowIfDisposed check in Reauthenticate in BinaryConnection. We can add the same exception handling there too for sameness, but it will be unreachable (at least for now).
There was a problem hiding this comment.
I think we should add same ODE handling. Or tests for it. It would be easy to introduce a dispose in the future and miss this update.
No description provided.