[
https://issues.apache.org/jira/browse/LOG4J2-1047?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14581645#comment-14581645
]
Gary Gregory commented on LOG4J2-1047:
--------------------------------------
Thank you for the new version of the patch. I can see what you are trying to do
more clearly now. I am not sure I like this kind of solution at first glance.
Making the inet address lazy loaded and mutable does not feel right to me. It
means we can construct an object that is always under construction.
Since the inet address is not the only thing that can go wrong, dealing with a
broken socket would make things more complicated. If the socket breaks, then we
need to resolve the host name again, since we might get a different IP address.
So when the socket breaks I need to clear the inet address too. More food for
thought!
A different higher level approach is considering the core config retrying
creating an appender n times at intervals m milliseconds.
If you have other appenders configured, they would get the log events but the
broken one would not. This would let the application run normally.
Another wrinkle is to allow the appender to fail so that a failover appender
can pick up the work. So the connect and reconnect behavior should be
configurable and not automatic. I'm pretty sure this patch does not make the
retry of inet address creation configurable, therefore not allowing a failover
appender to kick in...
Fun stuff. Requires more thought.
> When SyslogAppender can't find inetAddress at startup, it will never try to
> reconnect
> -------------------------------------------------------------------------------------
>
> Key: LOG4J2-1047
> URL: https://issues.apache.org/jira/browse/LOG4J2-1047
> Project: Log4j 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 2.3
> Environment: > lsb_release -a
> No LSB modules are available.
> Distributor ID: Ubuntu
> Description: Ubuntu 14.04.2 LTS
> Release: 14.04
> Codename: trusty
> > java -version
> java version "1.7.0_79"
> OpenJDK Runtime Environment (IcedTea 2.5.5) (7u79-2.5.5-0ubuntu0.14.04.2)
> OpenJDK 64-Bit Server VM (build 24.79-b02, mixed mode)
> Reporter: Guillaume Turri
> Priority: Minor
>
> * Steps to reproduce:
> ** Deactivate your wifi and unplug your network cable.
> ** Launch the following code with the following config
> ** Reactivate the network while the code is still running
> The code is:
> {code}
> import org.apache.logging.log4j.LogManager;
> import org.apache.logging.log4j.Logger
> public class App
> {
> private static final Logger _logger =
> LogManager.getLogger("TestLogger");
> public static void main( String[] args ) throws Exception
> {
> int id = 0;
> while ( true ){
> String msg = "message: " + id;
> _logger.error(msg);
> id++;
> Thread.sleep(1000);
> }
> }
> }
> {code}
> the config is:
> {code}
> <?xml version="1.0" encoding="UTF-8"?>
>
>
>
> <Configuration status="WARN">
> <Appenders>
> <Console name="Console" target="SYSTEM_OUT">
> <PatternLayout pattern="%d{HH:mm:ss.SSS} [%t] %-5level %logger{36} -
> %msg%n"/>
> </Console>
> <Syslog name="my-syslog" host="my-host" port="12201" protocol="TCP"
> format="RFC5424" mdcId="mdc" includeMDC="true" appName="testGTApp"
> newLine="true" connectTimeoutMillis="100" immediateFail="true"
> reconnectionDelayMillis="3000" immediateFlush="true" />
> </Appenders>
> <Loggers>
> <Root level="info">
> <AppenderRef ref="Console" />
> <AppenderRef ref="my-syslog"/>
> </Root>
> </Loggers>
> </Configuration>
> {code}
> where "my-host" is a host with an actual syslog listening on the given port
> and with the given protocol.
> "my-host" should be the name of the host. ie: it should *not* be an IP. It
> should *not* be localhost either.
> * Expected behavior: after we retrieve the network, messages are logged in
> the syslog
> * Actual behavior: it starts by logging on the console the following error,
> and then, it never connects to the syslog
> {code}
> 2015-06-09 16:31:26,939 ERROR Could not find address ofmy-host
> java.net.UnknownHostException: my-host: Name or service not known
> at java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
> at java.net.InetAddress$1.lookupAllHostAddr(InetAddress.java:901)
> at
> java.net.InetAddress.getAddressesFromNameService(InetAddress.java:1293)
> at java.net.InetAddress.getAllByName0(InetAddress.java:1246)
> at java.net.InetAddress.getAllByName(InetAddress.java:1162)
> at java.net.InetAddress.getAllByName(InetAddress.java:1098)
> at java.net.InetAddress.getByName(InetAddress.java:1048)
> at
> org.apache.logging.log4j.core.net.TcpSocketManager$TcpSocketManagerFactory.createManager(TcpSocketManager.java:279)
> at
> org.apache.logging.log4j.core.net.TcpSocketManager$TcpSocketManagerFactory.createManager(TcpSocketManager.java:272)
> at
> org.apache.logging.log4j.core.appender.AbstractManager.getManager(AbstractManager.java:71)
> at
> org.apache.logging.log4j.core.appender.OutputStreamManager.getManager(OutputStreamManager.java:60)
> at
> org.apache.logging.log4j.core.net.TcpSocketManager.getSocketManager(TcpSocketManager.java:114)
> at
> org.apache.logging.log4j.core.appender.SocketAppender.createSocketManager(SocketAppender.java:173)
> at
> org.apache.logging.log4j.core.appender.SyslogAppender.createAppender(SyslogAppender.java:145)
> [... truncated ...]
> 2015-06-09 16:31:26,942 ERROR Unable to invoke factory method in class class
> org.apache.logging.log4j.core.appender.SyslogAppender for element Syslog.
> java.lang.reflect.InvocationTargetException
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:606)
> at
> org.apache.logging.log4j.core.config.plugins.util.PluginBuilder.build(PluginBuilder.java:137)
> [... truncated. ...]
> 2015-06-09 16:31:26,944 ERROR Null object returned for Syslog in Appenders.
> 2015-06-09 16:31:26,947 ERROR Unable to locate appender my-syslog for logger
> {code}
> * Potential fix:
> The following patch fixes the issue. (it changes a few public signatures, so
> you may not want to apply it as-is).
> {code}
> From 0d2818762cf768e80f46fd08f48abac37747825b Mon Sep 17 00:00:00 2001
> From: Guillaume Turri <[email protected]>
> Date: Fri, 5 Jun 2015 17:44:15 +0200
> Subject: [PATCH] Syslog appender can catch up if we start without connection
> Moreover this patch removes duplication:
> * minor code duplication between DatagramSocketManager and TcpSocketManager
> in the way to handle the error when retrieving the InetAddress
> * data duplication in the AbstractSocketManager constructor, because we
> had to provide both a InetAddress and a host name, which both match the same
> thing
> ---
> .../logging/log4j/core/net/AbstractSocketManager.java | 19
> +++++++++++++++----
> .../logging/log4j/core/net/DatagramSocketManager.java | 16 +++-------------
> .../logging/log4j/core/net/SslSocketManager.java | 4 ++--
> .../logging/log4j/core/net/TcpSocketManager.java | 19
> +++++--------------
> 4 files changed, 25 insertions(+), 33 deletions(-)
> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/AbstractSocketManager.java
>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/AbstractSocketManager.java
> index 5ff43a3..4b0923c 100644
> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/AbstractSocketManager.java
> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/AbstractSocketManager.java
> @@ -19,6 +19,7 @@ package org.apache.logging.log4j.core.net;
> import java.io.OutputStream;
> import java.io.Serializable;
> import java.net.InetAddress;
> +import java.net.UnknownHostException;
> import java.util.HashMap;
> import java.util.Map;
>
> @@ -33,7 +34,14 @@ public abstract class AbstractSocketManager extends
> OutputStreamManager {
> /**
> * The Internet address of the host.
> */
> - protected final InetAddress inetAddress;
> + private InetAddress inetAddress;
> +
> + protected InetAddress getInetAddress() throws UnknownHostException{
> + if ( inetAddress == null ){
> + inetAddress = InetAddress.getByName(host);
> + }
> + return inetAddress;
> + }
>
> /**
> * The name of the host.
> @@ -49,15 +57,18 @@ public abstract class AbstractSocketManager extends
> OutputStreamManager {
> * The Constructor.
> * @param name The unique name of this connection.
> * @param os The OutputStream to manage.
> - * @param inetAddress The Internet address.
> * @param host The target host name.
> * @param port The target port number.
> */
> - public AbstractSocketManager(final String name, final OutputStream os,
> final InetAddress inetAddress, final String host,
> + public AbstractSocketManager(final String name, final OutputStream os,
> final String host,
> final int port, final Layout<? extends
> Serializable> layout) {
> super(os, name, layout);
> - this.inetAddress = inetAddress;
> this.host = host;
> + try {
> + this.inetAddress = getInetAddress();
> + } catch(UnknownHostException ex) {
> + LOGGER.error("Could not find address of " + this.host, ex);
> + }
> this.port = port;
> }
>
> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/DatagramSocketManager.java
>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/DatagramSocketManager.java
> index e16c6ef..5b0ab8f 100644
> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/DatagramSocketManager.java
> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/DatagramSocketManager.java
> @@ -18,8 +18,6 @@ package org.apache.logging.log4j.core.net;
>
> import java.io.OutputStream;
> import java.io.Serializable;
> -import java.net.InetAddress;
> -import java.net.UnknownHostException;
> import java.util.HashMap;
> import java.util.Map;
>
> @@ -38,14 +36,13 @@ public class DatagramSocketManager extends
> AbstractSocketManager {
> * The Constructor.
> * @param name The unique name of the connection.
> * @param os The OutputStream.
> - * @param inetAddress
> * @param host The host to connect to.
> * @param port The port on the host.
> * @param layout The layout
> */
> - protected DatagramSocketManager(final String name, final OutputStream
> os, final InetAddress inetAddress, final String host,
> + protected DatagramSocketManager(final String name, final OutputStream
> os, final String host,
> final int port, final Layout<? extends Serializable> layout)
> {
> - super(name, os, inetAddress, host, port, layout);
> + super(name, os, host, port, layout);
> }
>
> /**
> @@ -105,16 +102,9 @@ public class DatagramSocketManager extends
> AbstractSocketManager {
>
> @Override
> public DatagramSocketManager createManager(final String name, final
> FactoryData data) {
> - InetAddress inetAddress;
> - try {
> - inetAddress = InetAddress.getByName(data.host);
> - } catch (final UnknownHostException ex) {
> - LOGGER.error("Could not find address of " + data.host, ex);
> - return null;
> - }
> final OutputStream os = new DatagramOutputStream(data.host,
> data.port, data.layout.getHeader(),
> data.layout.getFooter());
> - return new DatagramSocketManager(name, os, inetAddress,
> data.host, data.port, data.layout);
> + return new DatagramSocketManager(name, os, data.host, data.port,
> data.layout);
> }
> }
> }
> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> index 87e7867..cc43490 100644
> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java
> @@ -57,10 +57,10 @@ public class SslSocketManager extends TcpSocketManager {
> * @param layout The Layout.
> */
> public SslSocketManager(final String name, final OutputStream os, final
> Socket sock,
> - final SslConfiguration sslConfig, final InetAddress inetAddress,
> final String host, final int port,
> + final SslConfiguration sslConfig, final String host, final int
> port,
> int connectTimeoutMillis, final int delay, final boolean
> immediateFail,
> final Layout<? extends Serializable> layout) {
> - super(name, os, sock, inetAddress, host, port, connectTimeoutMillis,
> delay, immediateFail, layout);
> + super(name, os, sock, host, port, connectTimeoutMillis, delay,
> immediateFail, layout);
> this.sslConfig = sslConfig;
> }
>
> diff --git
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
>
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
> index 9ab9f5f..20796bb 100644
> ---
> a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
> +++
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
> @@ -24,7 +24,6 @@ import java.net.ConnectException;
> import java.net.InetAddress;
> import java.net.InetSocketAddress;
> import java.net.Socket;
> -import java.net.UnknownHostException;
> import java.util.HashMap;
> import java.util.Map;
> import java.util.concurrent.CountDownLatch;
> @@ -67,7 +66,6 @@ public class TcpSocketManager extends AbstractSocketManager
> {
> * @param name The unique name of this connection.
> * @param os The OutputStream.
> * @param sock The Socket.
> - * @param inetAddress The Internet address of the host.
> * @param host The name of the host.
> * @param port The port number on the host.
> * @param connectTimeoutMillis the connect timeout in milliseconds.
> @@ -75,10 +73,10 @@ public class TcpSocketManager extends
> AbstractSocketManager {
> * @param immediateFail
> * @param layout The Layout.
> */
> - public TcpSocketManager(final String name, final OutputStream os, final
> Socket sock, final InetAddress inetAddress,
> + public TcpSocketManager(final String name, final OutputStream os, final
> Socket sock,
> final String host, final int port, int
> connectTimeoutMillis, final int delay,
> final boolean immediateFail, final Layout<?
> extends Serializable> layout) {
> - super(name, os, inetAddress, host, port, layout);
> + super(name, os, host, port, layout);
> this.connectTimeoutMillis = connectTimeoutMillis;
> this.reconnectionDelay = delay;
> this.socket = sock;
> @@ -205,7 +203,7 @@ public class TcpSocketManager extends
> AbstractSocketManager {
> while (!shutdown) {
> try {
> sleep(reconnectionDelay);
> - final Socket sock = createSocket(inetAddress, port);
> + final Socket sock = createSocket(getInetAddress(), port);
> final OutputStream newOS = sock.getOutputStream();
> synchronized (owner) {
> try {
> @@ -273,18 +271,11 @@ public class TcpSocketManager extends
> AbstractSocketManager {
> @Override
> public TcpSocketManager createManager(final String name, final
> FactoryData data) {
>
> - InetAddress inetAddress;
> OutputStream os;
> try {
> - inetAddress = InetAddress.getByName(data.host);
> - } catch (final UnknownHostException ex) {
> - LOGGER.error("Could not find address of " + data.host, ex);
> - return null;
> - }
> - try {
> final Socket socket = new Socket(data.host, data.port);
> os = socket.getOutputStream();
> - return new TcpSocketManager(name, os, socket, inetAddress,
> data.host, data.port,
> + return new TcpSocketManager(name, os, socket, data.host,
> data.port,
> data.connectTimeoutMillis, data.delayMillis,
> data.immediateFail, data.layout);
> } catch (final IOException ex) {
> LOGGER.error("TcpSocketManager (" + name + ") " + ex);
> @@ -293,7 +284,7 @@ public class TcpSocketManager extends
> AbstractSocketManager {
> if (data.delayMillis == 0) {
> return null;
> }
> - return new TcpSocketManager(name, os, null, inetAddress,
> data.host, data.port, data.connectTimeoutMillis,
> + return new TcpSocketManager(name, os, null, data.host,
> data.port, data.connectTimeoutMillis,
> data.delayMillis, data.immediateFail, data.layout);
> }
> }
> --
> 1.9.1
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
