Copilot commented on code in PR #6788:
URL: https://github.com/apache/ignite-3/pull/6788#discussion_r2433200719
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -164,6 +164,11 @@ private ClientSocket(
_ = RunReceiveLoop(_disposeTokenSource.Token);
}
+ /// <summary>
+ /// Finalizes an instance of the <see cref="ClientSocket"/> class.
+ /// </summary>
+ ~ClientSocket() => Dispose();
+
Review Comment:
[nitpick] Avoid calling Dispose() from a finalizer because it executes
managed-only cleanup (logging, timers, streams) on the finalizer thread. Use
the standard dispose pattern: implement Dispose(bool disposing), have the
finalizer call Dispose(false) and the public Dispose() call Dispose(true) +
GC.SuppressFinalize(this); skip logging/metrics and only release
unmanaged/native resources when disposing is false.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -998,18 +1005,44 @@ private void
HandlePartitionAssignmentChange(ResponseFlags flags, ref MsgPackRea
/// <summary>
/// Disposes this socket and completes active requests with the
specified exception.
/// </summary>
- /// <param name="ex">Exception that caused this socket to close. Null
when socket is closed by the user.</param>
+ /// <param name="ex">Exception that caused this socket to close. Null
when the socket is closed by the user.</param>
+ [SuppressMessage("Design", "CA1031:Do not catch general exception
types", Justification = "Reviewed.")]
private void Dispose(Exception? ex)
{
lock (_disposeLock)
{
+ // State check.
if (_disposeTokenSource.IsCancellationRequested)
{
return;
}
_disposeTokenSource.Cancel();
+ // Actual dispose.
+ try
+ {
+ _heartbeatTimer.Dispose();
+ }
+ catch (Exception e)
+ {
+ _logger.LogFailedSocketDispose(e);
+ }
+
+ try
+ {
+ _stream.Dispose();
+ }
+ catch (Exception e)
+ {
+ _logger.LogFailedSocketDispose(e);
+ }
Review Comment:
[nitpick] The dispose-and-log pattern is duplicated. Consider extracting a
small helper (e.g., SafeDispose(IDisposable? resource, string resourceName)) to
reduce duplication and ensure consistent logging of the resource name.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -298,6 +303,8 @@ public Task<PooledBuffer> DoOutInOpAsync(
/// <inheritdoc/>
public void Dispose()
{
+ GC.SuppressFinalize(this);
+
Dispose(null);
}
Review Comment:
[nitpick] Avoid calling Dispose() from a finalizer because it executes
managed-only cleanup (logging, timers, streams) on the finalizer thread. Use
the standard dispose pattern: implement Dispose(bool disposing), have the
finalizer call Dispose(false) and the public Dispose() call Dispose(true) +
GC.SuppressFinalize(this); skip logging/metrics and only release
unmanaged/native resources when disposing is false.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/LogMessages.cs:
##########
@@ -226,4 +226,11 @@ internal static partial void LogServerOpTrace(
EventId = 1030)]
internal static partial void LogFailedTableOpDebug(
this ILogger logger, Exception e, ClientOp op);
+
+ [LoggerMessage(
+ Message = "Failed to dispose socket",
+ Level = LogLevel.Warning,
+ EventId = 1031)]
+ internal static partial void LogFailedSocketDispose(
+ this ILogger logger, Exception e);
Review Comment:
The message 'Failed to dispose socket' is used for disposing both the
heartbeat timer and the stream in ClientSocket, which can mislead during
diagnostics. Consider adding a resource name parameter (e.g., Message =
\"Failed to dispose {resource}\") and include a string resource argument, or
add separate messages for 'Failed to dispose heartbeat timer' and 'Failed to
dispose network stream'.
```suggestion
Message = "Failed to dispose {Resource}",
Level = LogLevel.Warning,
EventId = 1031)]
internal static partial void LogFailedSocketDispose(
this ILogger logger, Exception e, string resource);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
