From fb8b42f19e2f0e49a9c161b4476da0ae86f31ffc Mon Sep 17 00:00:00 2001 From: Jan Friedrich Date: Tue, 15 Oct 2024 13:16:27 +0200 Subject: [PATCH] #194 fixed a bug in the Dispose-Logic of TelnetAppender --- scripts/build-preview.ps1 | 4 +- .../Appender/Internal/SimpleTelnetClient.cs | 52 ++++++ .../Appender/TelnetAppenderTest.cs | 92 ++++++++++ src/log4net/Appender/TelnetAppender.cs | 164 +++++++----------- 4 files changed, 209 insertions(+), 103 deletions(-) create mode 100644 src/log4net.Tests/Appender/Internal/SimpleTelnetClient.cs create mode 100644 src/log4net.Tests/Appender/TelnetAppenderTest.cs diff --git a/scripts/build-preview.ps1 b/scripts/build-preview.ps1 index 41429109..64b54d97 100644 --- a/scripts/build-preview.ps1 +++ b/scripts/build-preview.ps1 @@ -1,5 +1,5 @@ -$Version = '3.0.1' -$Preview = '2' +$Version = '3.0.2' +$Preview = '1' 'building ...' dotnet build -c Release "-p:GeneratePackages=true;PackageVersion=$Version-preview.$Preview" $PSScriptRoot/../src/log4net/log4net.csproj 'signing ...' diff --git a/src/log4net.Tests/Appender/Internal/SimpleTelnetClient.cs b/src/log4net.Tests/Appender/Internal/SimpleTelnetClient.cs new file mode 100644 index 00000000..0f9199f3 --- /dev/null +++ b/src/log4net.Tests/Appender/Internal/SimpleTelnetClient.cs @@ -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 +{ + /// + /// Telnet Client for unit testing + /// + /// Callback for received messages + /// TCP-Port to use + internal sealed class SimpleTelnetClient( + Action received, int port) : IDisposable + { + private readonly CancellationTokenSource cancellationTokenSource = new(); + private readonly TcpClient client = new(); + + /// + /// Runs the client (in a task) + /// + 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); + + /// + public void Dispose() + { + cancellationTokenSource.Cancel(); + cancellationTokenSource.Dispose(); + client.Dispose(); + } + } +} \ No newline at end of file diff --git a/src/log4net.Tests/Appender/TelnetAppenderTest.cs b/src/log4net.Tests/Appender/TelnetAppenderTest.cs new file mode 100644 index 00000000..683ae4f8 --- /dev/null +++ b/src/log4net.Tests/Appender/TelnetAppenderTest.cs @@ -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; + +/// +/// Tests for +/// +[TestFixture] +public sealed class TelnetAppenderTest +{ + /// + /// Simple Test für the + /// + /// + /// https://github.com/apache/logging-log4net/issues/194 + /// https://stackoverflow.com/questions/79053363/log4net-telnetappender-doesnt-work-after-migrate-to-log4net-3 + /// + [Test] + public void TelnetTest() + { + List received = []; + + XmlDocument log4netConfig = new(); + int port = 9090; + log4netConfig.LoadXml($""" + + + + + + + + + + + + +"""); + 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); + } + } + } +} \ No newline at end of file diff --git a/src/log4net/Appender/TelnetAppender.cs b/src/log4net/Appender/TelnetAppender.cs index 0e53edbe..3d11db34 100644 --- a/src/log4net/Appender/TelnetAppender.cs +++ b/src/log4net/Appender/TelnetAppender.cs @@ -33,10 +33,8 @@ namespace log4net.Appender /// /// /// - /// 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. /// /// @@ -47,8 +45,8 @@ namespace log4net.Appender /// Nicko Cadell public class TelnetAppender : AppenderSkeleton { - private SocketHandler? m_handler; - private int m_listeningPort = 23; + private SocketHandler? handler; + private int listeningPort = 23; /// /// The fully qualified type of the TelnetAppender class. @@ -75,18 +73,15 @@ public class TelnetAppender : AppenderSkeleton /// or greater than . 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; } /// @@ -115,31 +107,15 @@ protected override void OnClose() protected override bool RequiresLayout => true; /// - /// Initializes the appender based on the options set. - /// - /// - /// - /// This is part of the delayed object - /// activation scheme. The method must - /// be called on this object after the configuration properties have - /// been set. Until is called this - /// object is in an undefined state and must not be used. - /// - /// - /// If any of the configuration properties are modified then - /// must be called again. - /// - /// /// Create the socket handler and wait for connections - /// - /// + /// 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() /// The event to log. 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) /// /// /// - /// 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. /// /// protected class SocketHandler : IDisposable { private const int MAX_CONNECTIONS = 20; - private readonly Socket m_serverSocket; - private readonly List m_clients = new(); - private readonly object m_lockObj = new(); - private bool m_disposed; + private readonly Socket serverSocket; + private readonly List clients = new(); + private readonly object syncRoot = new(); + private bool wasDisposed; /// /// Class that represents a client connected to this handler /// protected class SocketClient : IDisposable { - private readonly Socket m_socket; - private readonly StreamWriter m_writer; + private readonly Socket socket; + private readonly StreamWriter writer; /// /// Create this for the specified @@ -198,11 +173,10 @@ protected class SocketClient : IDisposable /// 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) /// string to send public void Send(string message) { - m_writer.Write(message); - m_writer.Flush(); + writer.Write(message); + writer.Flush(); } /// @@ -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,16 +240,13 @@ public void Dispose() /// 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); /// /// Sends a string message to each of the connected clients. @@ -283,13 +254,12 @@ private void AcceptConnection() /// the text to send public void Send(string message) { - List localClients; - lock (m_lockObj) + lock (syncRoot) { - localClients = m_clients.ToList(); + localClients = clients.ToList(); } - + // Send outside lock. foreach (SocketClient client in localClients) { @@ -297,7 +267,7 @@ public void Send(string message) { 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) /// client to add private void AddClient(SocketClient client) { - lock (m_lockObj) + lock (syncRoot) { - m_clients.Add(client); + clients.Add(client); } } @@ -324,33 +294,17 @@ private void AddClient(SocketClient client) /// client to remove private void RemoveClient(SocketClient client) { - lock (m_lockObj) + lock (syncRoot) { - m_clients.Remove(client); + clients.Remove(client); } } /// /// Test if this handler has active connections /// - /// - /// true if this handler has active connections - /// - /// - /// - /// This property will be true while this handler has - /// active connections, that is at least one connection that - /// the handler will attempt to send a message to. - /// - /// - public bool HasConnections - { - get - { - // m_clients.Count is an atomic read that can be done outside the lock. - return m_clients.Count > 0; - } - } + public bool HasConnections => clients.Count > 0; + // clients.Count is an atomic read that can be done outside the lock. /// /// Callback used to accept a connection on the server socket @@ -364,20 +318,25 @@ public bool HasConnections /// 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 +356,10 @@ private void OnConnect(IAsyncResult asyncResult) } finally { - AcceptConnection(); + if (!wasDisposed) + { + AcceptConnection(); + } } } @@ -406,24 +368,24 @@ private void OnConnect(IAsyncResult asyncResult) /// 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 +394,7 @@ public void Dispose() try { - m_serverSocket.Dispose(); + serverSocket.Dispose(); } catch { @@ -442,4 +404,4 @@ public void Dispose() } } } -} +} \ No newline at end of file