Skip to content

Commit 024ecf0

Browse files
BorisDogBorisDog
authored
CSHARP-5361: Coverity analysis defect 162231, ServerMonitor._heartbeatCancellationTokenSource concurrent disposal
1 parent 5a7da05 commit 024ecf0

File tree

2 files changed

+96
-22
lines changed

2 files changed

+96
-22
lines changed

‎src/MongoDB.Driver/Core/Servers/ServerMonitor.cs

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* Copyright 2016-present MongoDB Inc.
1+
/* Copyright 2010-present MongoDB Inc.
22
*
33
* Licensed under the Apache License, Version 2.0 (the "License");
44
* you may not use this file except in compliance with the License.
@@ -128,18 +128,31 @@ public ServerMonitor(
128128
// public methods
129129
public void CancelCurrentCheck()
130130
{
131+
if (IsDisposed())
132+
{
133+
return;
134+
}
135+
131136
IConnection toDispose = null;
132137
lock (_lock)
133138
{
134139
if (!_heartbeatCancellationTokenSource.IsCancellationRequested)
135140
{
136-
_heartbeatCancellationTokenSource.Cancel();
137-
_heartbeatCancellationTokenSource.Dispose();
141+
// _heartbeatCancellationTokenSource instance might have been disposed in Dispose at this point
142+
TryCancelAndDispose(_heartbeatCancellationTokenSource);
143+
144+
if (IsDisposed()) // Skip creating new CancellationTokenSource if disposed
145+
{
146+
return;
147+
}
148+
138149
_heartbeatCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_monitorCancellationToken);
139150
// the previous hello or legacy hello cancellation token is still cancelled
140151

141152
toDispose = _connection;
142153
_connection = null;
154+
155+
_logger?.LogDebug(_serverId, "Heartbeat cancellation check requested");
143156
}
144157
}
145158
toDispose?.Dispose();
@@ -153,15 +166,13 @@ public void Dispose()
153166

154167
_monitorCancellationTokenSource.Cancel();
155168
_monitorCancellationTokenSource.Dispose();
156-
if (_connection != null)
157-
{
158-
_connection.Dispose();
159-
}
169+
170+
_connection?.Dispose();
160171
_roundTripTimeMonitor.Dispose();
161172
_heartbeatDelay?.Dispose();
162173

163-
_heartbeatCancellationTokenSource.Cancel();
164-
_heartbeatCancellationTokenSource.Dispose();
174+
// _heartbeatCancellationTokenSource instance might have been disposed in CancelCurrentCheck at this point.
175+
TryCancelAndDispose(_heartbeatCancellationTokenSource);
165176

166177
_logger?.LogDebug(_serverId, "Disposed");
167178
}
@@ -245,6 +256,8 @@ private CommandWireProtocol<BsonDocument> InitializeHelloProtocol(IConnection co
245256
return HelloHelper.CreateProtocol(helloCommand, _serverApi, commandResponseHandling);
246257
}
247258

259+
private bool IsDisposed() => _state.Value == State.Disposed;
260+
248261
private bool IsRunningInFaaS()
249262
{
250263
/* FaaS providers for each environment variable
@@ -523,7 +536,7 @@ private void SetDescription(ServerDescription oldDescription, ServerDescription
523536

524537
private void ThrowIfDisposed()
525538
{
526-
if (_state.Value == State.Disposed)
539+
if (IsDisposed())
527540
{
528541
throw new ObjectDisposedException(GetType().Name);
529542
}
@@ -538,6 +551,19 @@ private void ThrowIfNotOpen()
538551
}
539552
}
540553

554+
private void TryCancelAndDispose(CancellationTokenSource cancellationTokenSource)
555+
{
556+
try
557+
{
558+
cancellationTokenSource.Cancel();
559+
cancellationTokenSource.Dispose();
560+
}
561+
catch (ObjectDisposedException)
562+
{
563+
// Ignore ObjectDisposedException
564+
}
565+
}
566+
541567
// nested types
542568
private static class State
543569
{

‎tests/MongoDB.Driver.Tests/Core/Servers/ServerMonitorTests.cs

Lines changed: 60 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,34 @@ public ServerMonitorTests(ITestOutputHelper output) : base(output)
4949
{
5050
}
5151

52+
[Fact]
53+
public void CancelCurrentCheck_should_dispose_connection()
54+
{
55+
using var subject = CreateSubject(out _, out _, out _);
56+
57+
subject.CancelCurrentCheck();
58+
59+
subject._connection().Should().BeNull();
60+
61+
var logMessages = Logs.Where(l => l.Category.Contains(nameof(IServerMonitor))).ToArray();
62+
logMessages.Length.Should().Be(1);
63+
logMessages[0].Message.Contains("Heartbeat cancellation check requested").Should().BeTrue();
64+
}
65+
66+
[Fact]
67+
public void CancelCurrentCheck_should_do_nothing_if_disposed()
68+
{
69+
using var subject = CreateSubject(out _, out _, out _);
70+
71+
subject.Dispose();
72+
subject.CancelCurrentCheck();
73+
74+
var logMessages = Logs.Where(l => l.Category.Contains(nameof(IServerMonitor))).ToArray();
75+
logMessages.Length.Should().Be(2);
76+
logMessages[0].Message.Contains("Disposing").Should().BeTrue();
77+
logMessages[1].Message.Contains("Disposed").Should().BeTrue();
78+
}
79+
5280
[Fact]
5381
public void Constructor_should_throw_when_serverId_is_null()
5482
{
@@ -100,7 +128,7 @@ public void Constructor_should_throw_when_serverMonitorSettings_is_null()
100128
[Fact]
101129
public void Description_should_return_default_when_uninitialized()
102130
{
103-
var subject = CreateSubject(out _, out _, out _);
131+
using var subject = CreateSubject(out _, out _, out _);
104132

105133
var description = subject.Description;
106134

@@ -112,7 +140,7 @@ public void Description_should_return_default_when_uninitialized()
112140
[Fact]
113141
public void Description_should_return_default_when_disposed()
114142
{
115-
var subject = CreateSubject(out _, out _, out _);
143+
using var subject = CreateSubject(out _, out _, out _);
116144

117145
subject.Dispose();
118146

@@ -129,7 +157,7 @@ public void DescriptionChanged_should_be_raised_during_initial_handshake()
129157
var capturedEvents = new EventCapturer();
130158

131159
var changes = new List<ServerDescriptionChangedEventArgs>();
132-
var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
160+
using var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
133161
subject.DescriptionChanged += (o, e) => changes.Add(e);
134162

135163
SetupHeartbeatConnection(mockConnection);
@@ -157,7 +185,7 @@ public void Description_should_be_connected_after_successful_heartbeat()
157185
{
158186
var capturedEvents = new EventCapturer();
159187

160-
var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
188+
using var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
161189
SetupHeartbeatConnection(mockConnection);
162190
subject.Initialize();
163191
SpinWait.SpinUntil(() => subject.Description.State == ServerState.Connected, TimeSpan.FromSeconds(5)).Should().BeTrue();
@@ -175,7 +203,7 @@ public void Dispose_should_clear_all_resources_only_once()
175203
{
176204
var capturedEvents = new EventCapturer();
177205

178-
var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, captureConnectionEvents: true);
206+
using var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, captureConnectionEvents: true);
179207

180208
SetupHeartbeatConnection(mockConnection);
181209
subject.Initialize();
@@ -198,7 +226,7 @@ public void Heartbeat_should_make_immediate_next_attempt_for_streaming_protocol(
198226
.Capture<ServerHeartbeatSucceededEvent>()
199227
.Capture<ServerHeartbeatFailedEvent>()
200228
.Capture<ServerDescriptionChangedEvent>();
201-
var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTimeTripMonitor, capturedEvents);
229+
using var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTimeTripMonitor, capturedEvents);
202230

203231
subject.DescriptionChanged +=
204232
(o, e) =>
@@ -276,7 +304,7 @@ void AssertHeartbeatAttempt()
276304
[InlineData(true, true, "hello")]
277305
public void InitializeHelloProtocol_should_use_streaming_protocol_when_available(bool isStreamable, bool helloOk, string expectedCommand)
278306
{
279-
var subject = CreateSubject(out var mockConnection, out _, out _);
307+
using var subject = CreateSubject(out var mockConnection, out _, out _);
280308
SetupHeartbeatConnection(mockConnection, isStreamable, autoFillStreamingResponses: true);
281309

282310
mockConnection.WasReadTimeoutChanged.Should().Be(null);
@@ -304,7 +332,7 @@ public void RoundTripTimeMonitor_should_be_started_only_once_if_using_streaming_
304332
{
305333
var capturedEvents = new EventCapturer().Capture<ServerHeartbeatSucceededEvent>();
306334
var serverMonitorSettings = new ServerMonitorSettings(TimeSpan.FromSeconds(5), TimeSpan.FromMilliseconds(10));
307-
var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, serverMonitorSettings: serverMonitorSettings);
335+
using var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, serverMonitorSettings: serverMonitorSettings);
308336

309337
SetupHeartbeatConnection(mockConnection, isStreamable: true, autoFillStreamingResponses: false);
310338
mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage(), null);
@@ -328,7 +356,7 @@ public void RoundTripTimeMonitor_should_not_be_started_if_using_polling_protocol
328356
serverMonitoringMode: ServerMonitoringMode.Poll);
329357

330358
var capturedEvents = new EventCapturer().Capture<ServerHeartbeatSucceededEvent>();
331-
var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, serverMonitorSettings: serverMonitorSettings);
359+
using var subject = CreateSubject(out var mockConnection, out _, out var mockRoundTripTimeMonitor, capturedEvents, serverMonitorSettings: serverMonitorSettings);
332360

333361
SetupHeartbeatConnection(mockConnection, isStreamable: true, autoFillStreamingResponses: false);
334362
mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage());
@@ -346,7 +374,7 @@ public void RoundTripTimeMonitor_should_not_be_started_if_using_polling_protocol
346374
public void RequestHeartbeat_should_force_another_heartbeat()
347375
{
348376
var capturedEvents = new EventCapturer();
349-
var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
377+
using var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents);
350378

351379
SetupHeartbeatConnection(mockConnection);
352380
subject.Initialize();
@@ -364,6 +392,26 @@ public void RequestHeartbeat_should_force_another_heartbeat()
364392
capturedEvents.Any().Should().BeFalse();
365393
}
366394

395+
[Fact]
396+
public void RequestHeartbeat_should_throw_if_disposed()
397+
{
398+
var subject = CreateSubject(out var _, out _, out _);
399+
400+
subject.Dispose();
401+
402+
var exception = Record.Exception(() => subject.RequestHeartbeat());
403+
exception.Should().BeOfType<ObjectDisposedException>();
404+
}
405+
406+
[Fact]
407+
public void RequestHeartbeat_should_throw_if_not_open()
408+
{
409+
using var subject = CreateSubject(out var _, out _, out _);
410+
411+
var exception = Record.Exception(() => subject.RequestHeartbeat());
412+
exception.Should().BeOfType<InvalidOperationException>();
413+
}
414+
367415
[Fact]
368416
public void ServerHeartBeatEvents_should_not_be_awaited_if_using_polling_protocol()
369417
{
@@ -372,7 +420,7 @@ public void ServerHeartBeatEvents_should_not_be_awaited_if_using_polling_protoco
372420
.Capture<ServerHeartbeatSucceededEvent>();
373421

374422
var serverMonitorSettings = new ServerMonitorSettings(TimeSpan.FromSeconds(5), TimeSpan.FromMilliseconds(10), serverMonitoringMode: ServerMonitoringMode.Poll);
375-
var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents, serverMonitorSettings: serverMonitorSettings);
423+
using var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents, serverMonitorSettings: serverMonitorSettings);
376424

377425
SetupHeartbeatConnection(mockConnection, isStreamable: true, autoFillStreamingResponses: false);
378426
mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage());
@@ -473,7 +521,7 @@ public void Should_use_polling_protocol_if_running_in_FaaS_platform(string envir
473521
.Capture<ServerHeartbeatSucceededEvent>();
474522

475523
var serverMonitorSettings = new ServerMonitorSettings(TimeSpan.FromSeconds(5), TimeSpan.FromMilliseconds(10));
476-
var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents, serverMonitorSettings: serverMonitorSettings, environmentVariableProviderMock: environmentVariableProviderMock);
524+
using var subject = CreateSubject(out var mockConnection, out _, out _, capturedEvents, serverMonitorSettings: serverMonitorSettings, environmentVariableProviderMock: environmentVariableProviderMock);
477525

478526
SetupHeartbeatConnection(mockConnection, isStreamable: true, autoFillStreamingResponses: false);
479527
mockConnection.EnqueueCommandResponseMessage(CreateHeartbeatCommandResponseMessage());

0 commit comments

Comments
 (0)