This is an automated email from the ASF dual-hosted git repository. freeandnil pushed a commit to branch Feature/TelnetBug in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit d301fd7c5cf6ff766988acbd0707e7a6f1a08471 Author: Jan Friedrich <[email protected]> AuthorDate: Tue Oct 15 13:16:27 2024 +0200 #194 fixed a bug in the Dispose-Logic of TelnetAppender --- .../Appender/Internal/SimpleTelnetClient.cs | 52 +++++++ src/log4net.Tests/Appender/TelnetAppenderTest.cs | 92 ++++++++++++ src/log4net/Appender/TelnetAppender.cs | 156 ++++++++------------- 3 files changed, 206 insertions(+), 94 deletions(-) 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 +{ + /// <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(); + } + } +} \ 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; + +/// <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); + } + } + } +} \ No newline at end of file diff --git a/src/log4net/Appender/TelnetAppender.cs b/src/log4net/Appender/TelnetAppender.cs index 0e53edbe..18aa1a94 100644 --- a/src/log4net/Appender/TelnetAppender.cs +++ b/src/log4net/Appender/TelnetAppender.cs @@ -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 @@ namespace log4net.Appender /// 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 @@ namespace log4net.Appender { base.OnClose(); - if (m_handler is not null) - { - m_handler.Dispose(); - m_handler = null; - } + handler?.Dispose(); + handler = null; } /// <summary> @@ -115,31 +107,15 @@ namespace log4net.Appender 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 @@ namespace log4net.Appender /// <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 @@ namespace log4net.Appender /// </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 @@ namespace log4net.Appender /// </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 @@ namespace log4net.Appender /// <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 @@ namespace log4net.Appender { try { - m_writer.Dispose(); + writer.Dispose(); } catch { @@ -237,7 +211,7 @@ namespace log4net.Appender try { - m_socket.Shutdown(SocketShutdown.Both); + socket.Shutdown(SocketShutdown.Both); } catch { @@ -246,7 +220,7 @@ namespace log4net.Appender try { - m_socket.Dispose(); + socket.Dispose(); } catch { @@ -266,16 +240,13 @@ namespace log4net.Appender /// </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. @@ -283,13 +254,12 @@ namespace log4net.Appender /// <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) { @@ -297,7 +267,7 @@ namespace log4net.Appender { client.Send(message); } - catch (Exception) + catch { // The client has closed the connection, remove it from our list client.Dispose(); @@ -312,9 +282,9 @@ namespace log4net.Appender /// <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 @@ namespace log4net.Appender /// <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 @@ namespace log4net.Appender /// </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 @@ namespace log4net.Appender } finally { - AcceptConnection(); + if (!wasDisposed) + { + AcceptConnection(); + } } } @@ -406,24 +374,24 @@ namespace log4net.Appender /// </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 @@ namespace log4net.Appender try { - m_serverSocket.Dispose(); + serverSocket.Dispose(); } catch { @@ -442,4 +410,4 @@ namespace log4net.Appender } } } -} +} \ No newline at end of file
