CSHARP-6029: Make ICoreServerSessionPool disposable and send endSessions on dispose#1995
CSHARP-6029: Make ICoreServerSessionPool disposable and send endSessions on dispose#1995sanych-sun wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure server-side sessions are explicitly ended when a client/cluster is disposed (via an endSessions command), and adjusts test event capture behavior to avoid capturing disposal-time commands.
Changes:
- Added session-pool draining logic that issues
endSessionsfor pooled server sessions during cluster disposal. - Introduced
EventCapturer.Stop()and invoked it from the JSON-driven spec test runner. - Added a shared helper on
Clusterto select an available server and trigger session-pool cleanup during disposal.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Specifications/Runner/MongoClientJsonDrivenTestRunnerBase.cs | Stops event capture after test operations complete. |
| tests/MongoDB.Driver.TestHelpers/Core/EventCapturer.cs | Adds a stop flag to disable capturing after disposal begins. |
| src/MongoDB.Driver/Core/Clusters/SingleServerCluster.cs | Triggers session-pool cleanup before server removal on dispose. |
| src/MongoDB.Driver/Core/Clusters/MultiServerCluster.cs | Triggers session-pool cleanup during cluster disposal. |
| src/MongoDB.Driver/Core/Clusters/Cluster.cs | Adds ReleaseServerSessionPool() helper to drive cleanup. |
| src/MongoDB.Driver/Core/Bindings/ICoreServerSessionPool.cs | Extends pool contract with ReleaseAll(IServer). |
| src/MongoDB.Driver/Core/Bindings/CoreServerSessionPool.cs | Implements ReleaseAll to drain pooled sessions and send endSessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| sessionsBatch = _pool.GetRange(0, Math.Min(_pool.Count, 9999)); | ||
| _pool.RemoveRange(0, sessionsBatch.Count); | ||
| } |
| public void ReleaseAll(IServer server) | ||
| { | ||
| if (_pool.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| while (true) | ||
| { | ||
| List<ICoreServerSession> sessionsBatch; | ||
| lock (_lock) | ||
| { | ||
| if (_pool.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| sessionsBatch = _pool.GetRange(0, Math.Min(_pool.Count, 9999)); | ||
| _pool.RemoveRange(0, sessionsBatch.Count); | ||
| } | ||
|
|
||
| var endSessionCommand = new BsonDocument("endSessions", new BsonArray(sessionsBatch.Select(s => s.Id))); | ||
| foreach (var session in sessionsBatch) | ||
| { | ||
| session.Dispose(); | ||
| } | ||
|
|
||
| var operationContext = OperationContext.NoTimeout; | ||
| using var channel = server.GetChannel(operationContext); | ||
| channel.Command( | ||
| operationContext, | ||
| NoCoreSession.Instance, | ||
| ReadPreference.PrimaryPreferred, | ||
| DatabaseNamespace.Admin, | ||
| endSessionCommand, | ||
| null, | ||
| NoOpElementNameValidator.Instance, | ||
| null, | ||
| null, | ||
| CommandResponseHandling.Return, | ||
| BsonDocumentSerializer.Instance, | ||
| null); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore any errors. | ||
| } | ||
| } |
| internal interface ICoreServerSessionPool | ||
| { | ||
| internal interface ICoreServerSessionPool | ||
| { | ||
| ICoreServerSession AcquireSession(); | ||
| void ReleaseSession(ICoreServerSession serverSession); | ||
| } | ||
| ICoreServerSession AcquireSession(); | ||
| void ReleaseSession(ICoreServerSession serverSession); | ||
| void ReleaseAll(IServer server); | ||
| } |
| private readonly object _lock = new object(); | ||
| private readonly List<ICoreServerSession> _pool = new List<ICoreServerSession>(); | ||
| private readonly ILogger<LogCategories.Client> _logger; | ||
| private readonly ConcurrentStack<ICoreServerSession> _pool = new(); |
There was a problem hiding this comment.
Accordingly to spec pool should be LIFO.
| public void ReleaseSession(ICoreServerSession session) | ||
| { | ||
| lock (_lock) | ||
| ThrowIfDisposed(); |
There was a problem hiding this comment.
Is this be a behavioral breaking change? In the case server is disposed but the session/operation in flight is not, the exception might happen even for a successful operation?
There was a problem hiding this comment.
It is, but if MongoClient/Cluster is being disposed - all resources associated with it should be cleaned up. Otherwise we might let cluster "resurrect" and have leaked resources.
There was a problem hiding this comment.
Yes the resources should be clean up.
But should the pool throw on the "after-cleanup release attempt"?
There was a problem hiding this comment.
If somebody disposed client/cluster and then suddenly started returning sessions to the pool: it means a) they disposed cluster too early b) the returned sessions might not be closed properly (if CloseAndDispose is already done).
There was a problem hiding this comment.
As it was discussed offline we will log an error for now to prevent any possible breaking change in the minor release.
| } | ||
| else | ||
| { | ||
| Interlocked.Increment(ref _sessionsAcquired); |
There was a problem hiding this comment.
nit: consider having a single place for incrementing for readability (single return)
| { | ||
| _logger?.LogDebug( | ||
| "Closed server session pool for {clusterId} in {milliseconds}ms.", | ||
| _cluster.ClusterId, (Stopwatch.GetTimestamp() - timestamp) / (double)Stopwatch.Frequency * 1000); |
There was a problem hiding this comment.
Add how many session where disposed (out of initial)?
| } | ||
| catch(Exception ex) | ||
| { | ||
| _logger?.LogError(ex, "Error closing server session pool for {clusterId}: {exception}", _cluster.ClusterId, ex); |
There was a problem hiding this comment.
Do we need to pass ex as an message formatting argument as well?
There was a problem hiding this comment.
It's there, the last parameter.
There was a problem hiding this comment.
Yes, but do we need it twice?
| public void ReleaseSession(ICoreServerSession session) | ||
| { | ||
| lock (_lock) | ||
| ThrowIfDisposed(); |
There was a problem hiding this comment.
Yes the resources should be clean up.
But should the pool throw on the "after-cleanup release attempt"?
| subject.ReleaseSession(mockSession.Object); | ||
|
|
||
| if (releasedSessionWasRecentlyUsed == 1) | ||
| if(shouldReturnToPool) |
No description provided.