Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
#194 fixed a bug in the Dispose-Logic of TelnetAppender
Browse files Browse the repository at this point in the history
FreeAndNil committed Oct 15, 2024

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tvdeyen Thomas von Deyen
1 parent 8f3c3d4 commit d301fd7
Showing 3 changed files with 206 additions and 94 deletions.
52 changes: 52 additions & 0 deletions src/log4net.Tests/Appender/Internal/SimpleTelnetClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using System;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

namespace log4net.Tests.Appender.Internal
{
/// <summary>
/// Telnet Client for unit testing
/// </summary>
/// <param name="received">Callback for received messages</param>
/// <param name="port">TCP-Port to use</param>
internal sealed class SimpleTelnetClient(
Action<string> received, int port) : IDisposable
{
private readonly CancellationTokenSource cancellationTokenSource = new();
private readonly TcpClient client = new();

/// <summary>
/// Runs the client (in a task)
/// </summary>
internal void Run() => Task.Run(() =>
{
client.Connect(new IPEndPoint(IPAddress.Loopback, port));
// Get a stream object for reading and writing
using NetworkStream stream = client.GetStream();

int i;
byte[] bytes = new byte[256];

// Loop to receive all the data sent by the server
while ((i = stream.Read(bytes, 0, bytes.Length)) != 0)
{
string data = System.Text.Encoding.ASCII.GetString(bytes, 0, i);
received(data);
if (cancellationTokenSource.Token.IsCancellationRequested)
{
return;
}
}
}, cancellationTokenSource.Token);

/// <inheritdoc/>
public void Dispose()
{
cancellationTokenSource.Cancel();
cancellationTokenSource.Dispose();
client.Dispose();
}
}
}
92 changes: 92 additions & 0 deletions src/log4net.Tests/Appender/TelnetAppenderTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
#region Apache License
//
// Licensed to the Apache Software Foundation (ASF) under one or more
// contributor license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright ownership.
// The ASF licenses this file to you under the Apache License, Version 2.0
// (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
#endregion

using System;
using System.Collections.Generic;
using System.Threading;
using System.Xml;
using log4net.Appender;
using log4net.Config;
using log4net.Core;
using log4net.Repository;
using log4net.Tests.Appender.Internal;
using NUnit.Framework;

namespace log4net.Tests.Appender;

/// <summary>
/// Tests for <see cref="TelnetAppender"/>
/// </summary>
[TestFixture]
public sealed class TelnetAppenderTest
{
/// <summary>
/// Simple Test für the <see cref="TelnetAppender"/>
/// </summary>
/// <remarks>
/// https://github.com/apache/logging-log4net/issues/194
/// https://stackoverflow.com/questions/79053363/log4net-telnetappender-doesnt-work-after-migrate-to-log4net-3
/// </remarks>
[Test]
public void TelnetTest()
{
List<string> received = [];

XmlDocument log4netConfig = new();
int port = 9090;
log4netConfig.LoadXml($"""
<log4net>
<appender name="TelnetAppender" type="log4net.Appender.TelnetAppender">
<port value="{port}" />
<layout type="log4net.Layout.PatternLayout">
<conversionPattern value="%date %-5level - %message%newline" />
</layout>
</appender>
<root>
<level value="INFO"/>
<appender-ref ref="TelnetAppender"/>
</root>
</log4net>
""");
string logId = Guid.NewGuid().ToString();
ILoggerRepository repository = LogManager.CreateRepository(logId);
XmlConfigurator.Configure(repository, log4netConfig["log4net"]!);
using (SimpleTelnetClient telnetClient = new(Received, port))
{
telnetClient.Run();
WaitForReceived(1); // wait for welcome message
ILogger logger = repository.GetLogger("Telnet");
logger.Log(typeof(TelnetAppenderTest), Level.Info, logId, null);
WaitForReceived(2); // wait for log message
}
repository.Shutdown();
Assert.AreEqual(2, received.Count);
StringAssert.Contains(logId, received[1]);

void Received(string message) => received.Add(message);

void WaitForReceived(int count)
{
while (received.Count < count)
{
Thread.Sleep(10);
}
}
}
}
156 changes: 62 additions & 94 deletions src/log4net/Appender/TelnetAppender.cs
Original file line number Diff line number Diff line change
@@ -33,10 +33,8 @@ namespace log4net.Appender
/// </summary>
/// <remarks>
/// <para>
/// The TelnetAppender accepts socket connections and streams logging messages
/// back to the client.
/// The output is provided in a telnet-friendly way so that a log can be monitored
/// over a TCP/IP socket.
/// The TelnetAppender accepts socket connections and streams logging messages back to the client.
/// The output is provided in a telnet-friendly way so that a log can be monitored over a TCP/IP socket.
/// This allows simple remote monitoring of application logging.
/// </para>
/// <para>
@@ -47,8 +45,8 @@ namespace log4net.Appender
/// <author>Nicko Cadell</author>
public class TelnetAppender : AppenderSkeleton
{
private SocketHandler? m_handler;
private int m_listeningPort = 23;
private SocketHandler? handler;
private int listeningPort = 23;

/// <summary>
/// The fully qualified type of the TelnetAppender class.
@@ -75,18 +73,15 @@ public class TelnetAppender : AppenderSkeleton
/// or greater than <see cref="IPEndPoint.MaxPort" />.</exception>
public int Port
{
get => m_listeningPort;
get => listeningPort;
set
{
if (value < IPEndPoint.MinPort || value > IPEndPoint.MaxPort)
if (value is < IPEndPoint.MinPort or > IPEndPoint.MaxPort)
{
throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value,
$"The value specified for Port is less than {IPEndPoint.MinPort} or greater than {IPEndPoint.MaxPort}.");
}
else
{
m_listeningPort = value;
}
listeningPort = value;
}
}

@@ -102,11 +97,8 @@ protected override void OnClose()
{
base.OnClose();

if (m_handler is not null)
{
m_handler.Dispose();
m_handler = null;
}
handler?.Dispose();
handler = null;
}

/// <summary>
@@ -115,31 +107,15 @@ protected override void OnClose()
protected override bool RequiresLayout => true;

/// <summary>
/// Initializes the appender based on the options set.
/// </summary>
/// <remarks>
/// <para>
/// This is part of the <see cref="IOptionHandler"/> delayed object
/// activation scheme. The <see cref="ActivateOptions"/> method must
/// be called on this object after the configuration properties have
/// been set. Until <see cref="ActivateOptions"/> is called this
/// object is in an undefined state and must not be used.
/// </para>
/// <para>
/// If any of the configuration properties are modified then
/// <see cref="ActivateOptions"/> must be called again.
/// </para>
/// <para>
/// Create the socket handler and wait for connections
/// </para>
/// </remarks>
/// </summary>
public override void ActivateOptions()
{
base.ActivateOptions();
try
{
LogLog.Debug(declaringType, $"Creating SocketHandler to listen on port [{m_listeningPort}]");
m_handler = new SocketHandler(m_listeningPort);
LogLog.Debug(declaringType, $"Creating SocketHandler to listen on port [{listeningPort}]");
handler = new SocketHandler(listeningPort);
}
catch (Exception ex)
{
@@ -154,9 +130,9 @@ public override void ActivateOptions()
/// <param name="loggingEvent">The event to log.</param>
protected override void Append(LoggingEvent loggingEvent)
{
if (m_handler is not null && m_handler.HasConnections)
if (handler is not null && handler.HasConnections)
{
m_handler.Send(RenderLoggingEvent(loggingEvent));
handler.Send(RenderLoggingEvent(loggingEvent));
}
}

@@ -165,27 +141,26 @@ protected override void Append(LoggingEvent loggingEvent)
/// </summary>
/// <remarks>
/// <para>
/// The SocketHandler class is used to accept connections from
/// clients. It is threaded so that clients can connect/disconnect
/// asynchronously.
/// The SocketHandler class is used to accept connections from clients.
/// It is threaded so that clients can connect/disconnect asynchronously.
/// </para>
/// </remarks>
protected class SocketHandler : IDisposable
{
private const int MAX_CONNECTIONS = 20;

private readonly Socket m_serverSocket;
private readonly List<SocketClient> m_clients = new();
private readonly object m_lockObj = new();
private bool m_disposed;
private readonly Socket serverSocket;
private readonly List<SocketClient> clients = new();
private readonly object syncRoot = new();
private bool wasDisposed;

/// <summary>
/// Class that represents a client connected to this handler
/// </summary>
protected class SocketClient : IDisposable
{
private readonly Socket m_socket;
private readonly StreamWriter m_writer;
private readonly Socket socket;
private readonly StreamWriter writer;

/// <summary>
/// Create this <see cref="SocketClient"/> for the specified <see cref="Socket"/>
@@ -198,11 +173,10 @@ protected class SocketClient : IDisposable
/// </remarks>
public SocketClient(Socket socket)
{
m_socket = socket;

this.socket = socket;
try
{
m_writer = new StreamWriter(new NetworkStream(socket));
writer = new(new NetworkStream(socket));
}
catch
{
@@ -217,8 +191,8 @@ public SocketClient(Socket socket)
/// <param name="message">string to send</param>
public void Send(string message)
{
m_writer.Write(message);
m_writer.Flush();
writer.Write(message);
writer.Flush();
}

/// <summary>
@@ -228,7 +202,7 @@ public void Dispose()
{
try
{
m_writer.Dispose();
writer.Dispose();
}
catch
{
@@ -237,7 +211,7 @@ public void Dispose()

try
{
m_socket.Shutdown(SocketShutdown.Both);
socket.Shutdown(SocketShutdown.Both);
}
catch
{
@@ -246,7 +220,7 @@ public void Dispose()

try
{
m_socket.Dispose();
socket.Dispose();
}
catch
{
@@ -266,38 +240,34 @@ public void Dispose()
/// </remarks>
public SocketHandler(int port)
{
m_serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
m_serverSocket.Bind(new IPEndPoint(IPAddress.Any, port));
m_serverSocket.Listen(5);
serverSocket = new(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
serverSocket.Bind(new IPEndPoint(IPAddress.Any, port));
serverSocket.Listen(5);
AcceptConnection();
}

private void AcceptConnection()
{
m_serverSocket.BeginAccept(OnConnect, null);
}
private void AcceptConnection() => serverSocket.BeginAccept(OnConnect, null);

/// <summary>
/// Sends a string message to each of the connected clients.
/// </summary>
/// <param name="message">the text to send</param>
public void Send(string message)
{

List<SocketClient> localClients;
lock (m_lockObj)
lock (syncRoot)
{
localClients = m_clients.ToList();
localClients = clients.ToList();
}

// Send outside lock.
foreach (SocketClient client in localClients)
{
try
{
client.Send(message);
}
catch (Exception)
catch
{
// The client has closed the connection, remove it from our list
client.Dispose();
@@ -312,9 +282,9 @@ public void Send(string message)
/// <param name="client">client to add</param>
private void AddClient(SocketClient client)
{
lock (m_lockObj)
lock (syncRoot)
{
m_clients.Add(client);
clients.Add(client);
}
}

@@ -324,31 +294,21 @@ private void AddClient(SocketClient client)
/// <param name="client">client to remove</param>
private void RemoveClient(SocketClient client)
{
lock (m_lockObj)
lock (syncRoot)
{
m_clients.Remove(client);
clients.Remove(client);
}
}

/// <summary>
/// Test if this handler has active connections
/// </summary>
/// <value>
/// <c>true</c> if this handler has active connections
/// </value>
/// <remarks>
/// <para>
/// This property will be <c>true</c> while this handler has
/// active connections, that is at least one connection that
/// the handler will attempt to send a message to.
/// </para>
/// </remarks>
public bool HasConnections
{
get
{
// m_clients.Count is an atomic read that can be done outside the lock.
return m_clients.Count > 0;
return clients.Count > 0;
}
}

@@ -364,20 +324,25 @@ public bool HasConnections
/// </remarks>
private void OnConnect(IAsyncResult asyncResult)
{
if (wasDisposed)
{
return;
}

try
{
// Block until a client connects
Socket socket = m_serverSocket.EndAccept(asyncResult);
Socket socket = serverSocket.EndAccept(asyncResult);
LogLog.Debug(declaringType, $"Accepting connection from [{socket.RemoteEndPoint}]");
var client = new SocketClient(socket);

// m_clients.Count is an atomic read that can be done outside the lock.
int currentActiveConnectionsCount = m_clients.Count;
int currentActiveConnectionsCount = clients.Count;
if (currentActiveConnectionsCount < MAX_CONNECTIONS)
{
try
{
client.Send($"TelnetAppender v1.0 ({(currentActiveConnectionsCount + 1)} active connections)\r\n\r\n");
client.Send($"TelnetAppender v1.0 ({currentActiveConnectionsCount + 1} active connections)\r\n\r\n");
AddClient(client);
}
catch
@@ -397,7 +362,10 @@ private void OnConnect(IAsyncResult asyncResult)
}
finally
{
AcceptConnection();
if (!wasDisposed)
{
AcceptConnection();
}
}
}

@@ -406,24 +374,24 @@ private void OnConnect(IAsyncResult asyncResult)
/// </summary>
public void Dispose()
{
if (m_disposed)
if (wasDisposed)
{
return;
}

m_disposed = true;
wasDisposed = true;

lock (m_lockObj)
lock (syncRoot)
{
foreach (SocketClient client in m_clients)
foreach (SocketClient client in clients)
{
client.Dispose();
}
m_clients.Clear();
clients.Clear();

try
{
m_serverSocket.Shutdown(SocketShutdown.Both);
serverSocket.Shutdown(SocketShutdown.Both);
}
catch
{
@@ -432,7 +400,7 @@ public void Dispose()

try
{
m_serverSocket.Dispose();
serverSocket.Dispose();
}
catch
{
@@ -442,4 +410,4 @@ public void Dispose()
}
}
}
}
}

0 comments on commit d301fd7

Please sign in to comment.