Skip to content
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.

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

Enable diagnostics in release #1263

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
11 changes: 11 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,17 @@ MA0053.exceptions_should_be_sealed = true
# Also report diagnostic for types that define (new) virtual members.
MA0053.class_with_virtual_member_shoud_be_sealed = true

# MA0076: Do not use implicit culture-sensitive ToString in interpolated strings
#
# Interpolated strings have performance benefits.
dotnet_diagnostic.MA0076.severity = none
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved

# MA0089: Optimize string method usage
#
# Duplicate of CA1865-CA1867 /
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved
# For string.Join, results in verbose code for very little gain.
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved
dotnet_diagnostic.MA0089.severity = none

# MA0112: Use 'Count > 0' instead of 'Any()'
#
# This rule is disabled by default, hence we need to explicitly enable it.
Expand Down
5 changes: 2 additions & 3 deletions src/Renci.SshNet/BaseClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using System.Threading;
using System.Threading.Tasks;

using Renci.SshNet.Abstractions;
using Renci.SshNet.Common;
using Renci.SshNet.Messages.Transport;

Expand Down Expand Up @@ -323,7 +322,7 @@ public async Task ConnectAsync(CancellationToken cancellationToken)
/// <exception cref="ObjectDisposedException">The method was called after the client was disposed.</exception>
public void Disconnect()
{
DiagnosticAbstraction.Log("Disconnecting client.");
Diagnostics.Log("Disconnecting client.", System.Diagnostics.TraceEventType.Verbose);

CheckDisposed();

Expand Down Expand Up @@ -422,7 +421,7 @@ protected virtual void Dispose(bool disposing)

if (disposing)
{
DiagnosticAbstraction.Log("Disposing client.");
Diagnostics.Log("Disposing client.", System.Diagnostics.TraceEventType.Verbose);

Disconnect();

Expand Down
6 changes: 3 additions & 3 deletions src/Renci.SshNet/Channels/Channel.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.Net.Sockets;
using System.Threading;

using Renci.SshNet.Abstractions;
using Renci.SshNet.Common;
using Renci.SshNet.Messages;
using Renci.SshNet.Messages.Connection;
Expand Down Expand Up @@ -554,9 +554,9 @@ protected virtual void Close()
// SSH_MSG_CHANNEL_CLOSE as response to a SSH_MSG_CHANNEL_CLOSE sent by the
// server
var closeWaitResult = _session.TryWait(_channelClosedWaitHandle, ConnectionInfo.ChannelCloseTimeout);
if (closeWaitResult != WaitResult.Success)
if (closeWaitResult != WaitResult.Success && Diagnostics.IsEnabled(TraceEventType.Warning))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get why we need the condition Diagnostics.IsEnabled(TraceEventType.Warning) outside the log method.

what if someone forgot to check it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need it, it just prevents doing any extra work like allocating string instances unnecessarily in the case that the log level is not enabled. But TBH I don't think it would make a material impact either way.

{
DiagnosticAbstraction.Log(string.Format("Wait for channel close not successful: {0:G}.", closeWaitResult));
Diagnostics.Log($"Wait for channel close not successful: {closeWaitResult}.", TraceEventType.Warning);
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/Renci.SshNet/Channels/ChannelDirectTcpip.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;
using System.Threading;

using Renci.SshNet.Abstractions;
using Renci.SshNet.Common;
using Renci.SshNet.Messages.Connection;
Expand Down Expand Up @@ -156,8 +158,10 @@ private void ShutdownSocket(SocketShutdown how)
}
catch (SocketException ex)
{
// TODO: log as warning
DiagnosticAbstraction.Log("Failure shutting down socket: " + ex);
if (Diagnostics.IsEnabled(TraceEventType.Warning))
{
Diagnostics.Log("Failure shutting down socket: " + ex, TraceEventType.Warning);
}
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/Renci.SshNet/Channels/ChannelForwardedTcpip.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using System;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;

using Renci.SshNet.Abstractions;
using Renci.SshNet.Common;
using Renci.SshNet.Messages.Connection;
Expand Down Expand Up @@ -138,8 +140,10 @@ private void ShutdownSocket(SocketShutdown how)
}
catch (SocketException ex)
{
// TODO: log as warning
DiagnosticAbstraction.Log("Failure shutting down socket: " + ex);
if (Diagnostics.IsEnabled(TraceEventType.Warning))
{
Diagnostics.Log("Failure shutting down socket: " + ex, TraceEventType.Warning);
}
}
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/Renci.SshNet/Connection/ConnectorBase.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;
using System.Threading;
Expand Down Expand Up @@ -41,7 +42,10 @@ protected Socket SocketConnect(string host, int port, TimeSpan timeout)
var ipAddress = DnsAbstraction.GetHostAddresses(host)[0];
var ep = new IPEndPoint(ipAddress, port);

DiagnosticAbstraction.Log(string.Format("Initiating connection to '{0}:{1}'.", host, port));
if (Diagnostics.IsEnabled(TraceEventType.Information))
{
Diagnostics.Log($"Initiating connection to '{host}:{port}'.", TraceEventType.Information);
}

var socket = SocketFactory.Create(ep.AddressFamily, SocketType.Stream, ProtocolType.Tcp);

Expand Down Expand Up @@ -76,7 +80,10 @@ protected async Task<Socket> SocketConnectAsync(string host, int port, Cancellat
var ipAddress = (await DnsAbstraction.GetHostAddressesAsync(host).ConfigureAwait(false))[0];
var ep = new IPEndPoint(ipAddress, port);

DiagnosticAbstraction.Log(string.Format("Initiating connection to '{0}:{1}'.", host, port));
if (Diagnostics.IsEnabled(TraceEventType.Information))
{
Diagnostics.Log($"Initiating connection to '{host}:{port}'.", TraceEventType.Information);
}

var socket = SocketFactory.Create(ep.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
try
Expand Down
Original file line number Diff line number Diff line change
@@ -1,28 +1,24 @@
using System.ComponentModel;
using System;
using System.Diagnostics;

namespace Renci.SshNet.Abstractions
namespace Renci.SshNet
{
/// <summary>
/// Provides access to the <see cref="System.Diagnostics"/> internals of SSH.NET.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DiagnosticAbstraction
public static class Diagnostics
{
/// <summary>
/// The <see cref="TraceSource"/> instance used by SSH.NET.
/// </summary>
/// <remarks>
/// <para>
/// Currently, the library only traces events when compiled in Debug mode.
/// </para>
/// <para>
/// Configuration on .NET Core must be done programmatically, e.g.
/// <code>
/// DiagnosticAbstraction.Source.Switch = new SourceSwitch("sourceSwitch", "Verbose");
/// DiagnosticAbstraction.Source.Listeners.Remove("Default");
/// DiagnosticAbstraction.Source.Listeners.Add(new ConsoleTraceListener());
/// DiagnosticAbstraction.Source.Listeners.Add(new TextWriterTraceListener("trace.log"));
/// Diagnostics.Source.Switch = new SourceSwitch("sourceSwitch", nameof(SourceLevels.Verbose));
Rob-Hague marked this conversation as resolved.
Show resolved Hide resolved
/// Diagnostics.Source.Listeners.Remove("Default");
/// Diagnostics.Source.Listeners.Add(new ConsoleTraceListener());
/// Diagnostics.Source.Listeners.Add(new TextWriterTraceListener("SshNetTrace.log"));
/// </code>
/// </para>
/// <para>
Expand Down Expand Up @@ -53,17 +49,19 @@ public static class DiagnosticAbstraction
public static readonly TraceSource Source = new TraceSource("SshNet.Logging");

/// <summary>
/// Logs a message to <see cref="Source"/> at the <see cref="TraceEventType.Verbose"/>
/// level.
/// Logs a message to <see cref="Source"/> with the specified event type.
/// </summary>
/// <param name="text">The message to log.</param>
/// <param name="type">The trace event type.</param>
[Conditional("DEBUG")]
public static void Log(string text, TraceEventType type = TraceEventType.Verbose)
/// <param name="eventType">The trace event type.</param>
public static void Log(string text, TraceEventType eventType)
{
Source.TraceEvent(eventType, Environment.CurrentManagedThreadId, text);
}

/// <inheritdoc cref="SourceSwitch.ShouldTrace(TraceEventType)" />
public static bool IsEnabled(TraceEventType eventType)
{
Source.TraceEvent(type,
System.Environment.CurrentManagedThreadId,
text);
return Source.Switch.ShouldTrace(eventType);
}
}
}
4 changes: 2 additions & 2 deletions src/Renci.SshNet/ForwardedPortDynamic.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Net;
Expand Down Expand Up @@ -413,8 +414,7 @@ private void InternalStop(TimeSpan timeout)

if (!_pendingChannelCountdown.Wait(timeout))
{
// TODO: log as warning
DiagnosticAbstraction.Log("Timeout waiting for pending channels in dynamic forwarded port to close.");
Diagnostics.Log("Timeout waiting for pending channels in dynamic forwarded port to close.", TraceEventType.Warning);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Renci.SshNet/ForwardedPortLocal.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;
using System.Threading;
Expand Down Expand Up @@ -402,8 +403,7 @@ private void InternalStop(TimeSpan timeout)

if (!_pendingChannelCountdown.Wait(timeout))
{
// TODO: log as warning
DiagnosticAbstraction.Log("Timeout waiting for pending channels in local forwarded port to close.");
Diagnostics.Log("Timeout waiting for pending channels in local forwarded port to close.", TraceEventType.Warning);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Renci.SshNet/ForwardedPortRemote.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.Net;
using System.Threading;
Expand Down Expand Up @@ -216,8 +217,7 @@ protected override void StopPort(TimeSpan timeout)

if (!_pendingChannelCountdown.Wait(timeout))
{
// TODO: log as warning
DiagnosticAbstraction.Log("Timeout waiting for pending channels in remote forwarded port to close.");
Diagnostics.Log("Timeout waiting for pending channels in remote forwarded port to close.", TraceEventType.Warning);
}

_status = ForwardedPortStatus.Stopped;
Expand Down
6 changes: 6 additions & 0 deletions src/Renci.SshNet/Messages/Transport/DisconnectMessage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,5 +95,11 @@ internal override void Process(Session session)
{
session.OnDisconnectReceived(this);
}

/// <inheritdoc/>
public override string ToString()
{
return $"SSH_MSG_DISCONNECT ({ReasonCode}) {Description}";
}
}
}
Loading