Skip to content

CSHARP-5798: Implement test to track if any UnobservedTaskExceptions were raised while test run #1836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sanych-sun merged 5 commits into from
Dec 9, 2025
Merged

CSHARP-5798: Implement test to track if any UnobservedTaskExceptions were raised while test run #1836

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b9eb50
CSHARP-5798: Implement test to track if any UnobservedTaskExceptions …
sanych-sun Dec 8, 2025
83e8b52
pr
sanych-sun Dec 8, 2025
1c3449e
CSHARP-5798: Implement test to track if any UnobservedTaskExceptions …
sanych-sun Dec 9, 2025
b8ab963
PR
sanych-sun Dec 9, 2025
823a0f7
pr
sanych-sun Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/MongoDB.Driver/Core/Connections/TcpStreamFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ namespace MongoDB.Driver.Core.Connections
/// </summary>
internal sealed class TcpStreamFactory : IStreamFactory
{
private static readonly byte[] __ensureConnectedBuffer = new byte[1];

// fields
private readonly TcpStreamSettings _settings;

Expand Down Expand Up @@ -165,10 +167,18 @@ private void ConfigureConnectedSocket(Socket socket)

private void Connect(Socket socket, EndPoint endPoint, CancellationToken cancellationToken)
{
var isSocketDisposed = false;
var callbackState = new ConnectOperationState(socket);
using var timeoutCancellationTokenSource = new CancellationTokenSource(_settings.ConnectTimeout);
using var combinedCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token);
using var cancellationSubscription = combinedCancellationTokenSource.Token.Register(DisposeSocket);
using var cancellationSubscription = combinedCancellationTokenSource.Token.Register(state =>
{
var operationState = (ConnectOperationState)state;
if (operationState.IsSucceeded)
{
return;
}
DisposeSocket(operationState.Socket);
}, callbackState);
Comment on lines 171 to 178
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential race condition between setting callbackState.IsSucceeded = true on line 199 and checking operationState.IsSucceeded on line 176. If cancellation occurs between EnsureConnected completing and IsSucceeded being set, the socket could be disposed even though the connection succeeded. Consider setting IsSucceeded = true before calling EnsureConnected, or use a lock/interlocked operation to ensure thread-safe state updates.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@sanych-sun sanych-sun Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot be sure if the connection was really established until we done the aliveness check, so we cannot set the value before calling EnsureConnected.


try
{
Expand All @@ -185,13 +195,12 @@ private void Connect(Socket socket, EndPoint endPoint, CancellationToken cancell
#else
socket.Connect(endPoint);
#endif
EnsureConnected(socket);
callbackState.IsSucceeded = true;
}
catch
{
if (!isSocketDisposed)
{
DisposeSocket();
}
DisposeSocket(socket);

cancellationToken.ThrowIfCancellationRequested();
if (timeoutCancellationTokenSource.IsCancellationRequested)
Expand All @@ -202,9 +211,8 @@ private void Connect(Socket socket, EndPoint endPoint, CancellationToken cancell
throw;
}

void DisposeSocket()
static void DisposeSocket(Socket socket)
{
isSocketDisposed = true;
try
{
socket.Dispose();
Expand All @@ -214,6 +222,23 @@ void DisposeSocket()
// Ignore any exceptions.
}
}

static void EnsureConnected(Socket socket)
Copy link
Member Author

@sanych-sun sanych-sun Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation of the method is inspired by the recommented by Microsoft way to check if the socket is alive:
https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.connected?view=net-10.0

{
bool originalBlockingState = socket.Blocking;
Copy link
Contributor

@BorisDog BorisDog Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: var

socket.Blocking = false;

try
{
// Try to use the socket to ensure it's connected. On MacOS with net6.0 sometimes Connect is completed successfully even after the socket disposal.
socket.Send(__ensureConnectedBuffer, 0, 0);
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending 0 bytes may not reliably detect a disposed socket on all platforms. Consider documenting why this specific approach was chosen for the macOS .NET 6 issue, or explore alternative detection methods such as checking socket.Connected property or using socket.Poll() with a zero timeout. The current implementation's effectiveness should be validated across different platforms.

Suggested change
// Try to use the socket to ensure it's connected. On MacOS with net6.0 sometimes Connect is completed successfully even after the socket disposal.
socket.Send(__ensureConnectedBuffer, 0, 0);
// Use Poll and Connected to ensure the socket is actually connected.
// This is a cross-platform way to check for a disposed or disconnected socket.
if (!socket.Connected || !socket.Poll(0, SelectMode.SelectWrite))
{
throw new SocketException((int)SocketError.NotConnected);
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@sanych-sun sanych-sun Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree. Sending the 0-bytes is done as per Microsoft's suggestion. Also socket.Connected does not makes any checks, but returns results based on the last operation results. Problem with socket.Poll() - we need to provide a small-timeout, what value should we use?

Copy link
Member Author

@sanych-sun sanych-sun Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's OK. If ensuring the socket is alive takes too long - we want to interrupt the operation by disposing the socket.

}
finally
{
// Restore original blocking state
socket.Blocking = originalBlockingState;
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The socket's blocking state is not restored if accessing socket.Blocking throws an ObjectDisposedException. If the socket is already disposed when entering EnsureConnected, the getter for originalBlockingState will throw, but the finally block won't execute to restore the state (though this is moot since the socket is disposed). More critically, if the socket gets disposed between line 229 and the try block, the finally block will attempt to set socket.Blocking on a disposed socket. Consider wrapping the entire method body in a try-catch to handle ObjectDisposedException gracefully.

Suggested change
bool originalBlockingState = socket.Blocking;
socket.Blocking = false;
try
{
// Try to use the socket to ensure it's connected. On MacOS with net6.0 sometimes Connect is completed successfully even after the socket disposal.
socket.Send(__ensureConnectedBuffer, 0, 0);
}
finally
{
// Restore original blocking state
socket.Blocking = originalBlockingState;
try
{
bool originalBlockingState = socket.Blocking;
socket.Blocking = false;
try
{
// Try to use the socket to ensure it's connected. On MacOS with net6.0 sometimes Connect is completed successfully even after the socket disposal.
socket.Send(__ensureConnectedBuffer, 0, 0);
}
finally
{
// Restore original blocking state
socket.Blocking = originalBlockingState;
}
}
catch (ObjectDisposedException)
{
// Socket is already disposed, nothing to do.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@sanych-sun sanych-sun Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK for the method to throw ObjectDisposedExcpetion, we have try-catch in the outer code.

}
}
}

private async Task ConnectAsync(Socket socket, EndPoint endPoint, CancellationToken cancellationToken)
Expand Down Expand Up @@ -376,5 +401,10 @@ public int Compare(EndPoint x, EndPoint y)
return 0;
}
}

private sealed record ConnectOperationState(Socket Socket)
{
public bool IsSucceeded { get; set; }
}
}
}