You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2016/08/31 22:16:40 UTC

Memory leak in TCP appender in odd case

We have this code
in org.apache.logging.log4j.core.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String,
FactoryData):

        @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,
ex);
                return null;
            }
            try {
                // LOG4J2-1042
                final Socket socket = new Socket();
                socket.connect(new InetSocketAddress(data.host, data.port),
data.connectTimeoutMillis);
                os = socket.getOutputStream();
                return new TcpSocketManager(name, os, socket, inetAddress,
data.host, data.port,
                        data.connectTimeoutMillis, data.delayMillis,
data.immediateFail, data.layout);
            } catch (final IOException ex) {
                LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
                os = new ByteArrayOutputStream();
            }
            if (data.delayMillis == 0) {
                return null;
            }
            return new TcpSocketManager(name, os, null, inetAddress,
data.host, data.port, data.connectTimeoutMillis,
                    data.delayMillis, data.immediateFail, data.layout);
        }

Notice:

                LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
                os = new ByteArrayOutputStream();

What is the point of that? It creates an unbound stream in RAM, creating
one problem while dealing with another.

I think it would be better to not create the manager at all.

Thoughts?

Gary

-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Gary Gregory <ga...@gmail.com>.
Created LOG4J2-1562.

Gary

On Sat, Sep 3, 2016 at 5:06 PM, Remko Popma <re...@gmail.com> wrote:

> I'd say start with the NullOutputStream: that gives the same behavior we
> have now but removes the danger of running out of memory. That's an
> important improvement.
>
> Actually supporting message buffering until reconnect properly is not
> going to be trivial. (Wouldn't a spool file be better than a memory buffer?
> Bounded or unbounded? What to do if we discover an unsent spool file at
> startup? How do we handle disconnects while sending events from the spool
> file? What about log4j shutdown while sending a large spool file? Or
> process crashes?) We're entering Kafka-like territory here...
>
> Remko
> Sent from my iPhone
>
> On 2016/09/04, at 4:02, Gary Gregory <ga...@gmail.com> wrote:
>
> On Thu, Sep 1, 2016 at 11:40 PM, Remko Popma <re...@gmail.com>
> wrote:
>
>> I just had a look at the code but I can't find a place where there's any
>> attempt to get the bytes out of the ByteArrayOutputStream.
>> If we're going to drop the messages anyway we should use a
>> NullOutputStream.
>>
>
> So one fix would be for us to add and use a NullOutputStream class. The
> other would be to flush the temp stream to the new stream on reconnect.
>
> In one case, we loose data, in the other we potentially use a more RAM in
> an unbounded way, if the server never is there for example.
>
> There could be a third option where we use a bounded output stream.
>
> What to do?
>
> Gary
>
>>
>> Sent from my iPhone
>>
>> On 2016/09/02, at 12:46, Gary Gregory <ga...@gmail.com> wrote:
>>
>> Yeah, but if the server is never up, the array will keep on growing...
>>
>> Gary
>>
>> On Wed, Aug 31, 2016 at 4:47 PM, Remko Popma <re...@gmail.com>
>> wrote:
>>
>>> (Without looking at the code) I vaguely remember that class has a
>>> reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?
>>>
>>> Sent from my iPhone
>>>
>>> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
>>>
>>> We have this code in org.apache.logging.log4j.co
>>> re.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String,
>>> FactoryData):
>>>
>>>         @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, ex);
>>>                 return null;
>>>             }
>>>             try {
>>>                 // LOG4J2-1042
>>>                 final Socket socket = new Socket();
>>>                 socket.connect(new InetSocketAddress(data.host,
>>> data.port), data.connectTimeoutMillis);
>>>                 os = socket.getOutputStream();
>>>                 return new TcpSocketManager(name, os, socket,
>>> inetAddress, data.host, data.port,
>>>                         data.connectTimeoutMillis, data.delayMillis,
>>> data.immediateFail, data.layout);
>>>             } catch (final IOException ex) {
>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex,
>>> ex);
>>>                 os = new ByteArrayOutputStream();
>>>             }
>>>             if (data.delayMillis == 0) {
>>>                 return null;
>>>             }
>>>             return new TcpSocketManager(name, os, null, inetAddress,
>>> data.host, data.port, data.connectTimeoutMillis,
>>>                     data.delayMillis, data.immediateFail, data.layout);
>>>         }
>>>
>>> Notice:
>>>
>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex,
>>> ex);
>>>                 os = new ByteArrayOutputStream();
>>>
>>> What is the point of that? It creates an unbound stream in RAM, creating
>>> one problem while dealing with another.
>>>
>>> I think it would be better to not create the manager at all.
>>>
>>> Thoughts?
>>>
>>> Gary
>>>
>>> --
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>> Java Persistence with Hibernate, Second Edition
>>> <http://www.manning.com/bauer3/>
>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>>> Spring Batch in Action <http://www.manning.com/templier/>
>>> Blog: http://garygregory.wordpress.com
>>> Home: http://garygregory.com/
>>> Tweet! http://twitter.com/GaryGregory
>>>
>>>
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> Java Persistence with Hibernate, Second Edition
>> <http://www.manning.com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition
> <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Remko Popma <re...@gmail.com>.
I'd say start with the NullOutputStream: that gives the same behavior we have now but removes the danger of running out of memory. That's an important improvement. 

Actually supporting message buffering until reconnect properly is not going to be trivial. (Wouldn't a spool file be better than a memory buffer? Bounded or unbounded? What to do if we discover an unsent spool file at startup? How do we handle disconnects while sending events from the spool file? What about log4j shutdown while sending a large spool file? Or process crashes?) We're entering Kafka-like territory here...

Remko
Sent from my iPhone

> On 2016/09/04, at 4:02, Gary Gregory <ga...@gmail.com> wrote:
> 
>> On Thu, Sep 1, 2016 at 11:40 PM, Remko Popma <re...@gmail.com> wrote:
>> I just had a look at the code but I can't find a place where there's any attempt to get the bytes out of the ByteArrayOutputStream. 
>> If we're going to drop the messages anyway we should use a NullOutputStream. 
> 
> So one fix would be for us to add and use a NullOutputStream class. The other would be to flush the temp stream to the new stream on reconnect.
> 
> In one case, we loose data, in the other we potentially use a more RAM in an unbounded way, if the server never is there for example.
> 
> There could be a third option where we use a bounded output stream.
> 
> What to do?
> 
> Gary
>> 
>> Sent from my iPhone
>> 
>>> On 2016/09/02, at 12:46, Gary Gregory <ga...@gmail.com> wrote:
>>> 
>>> Yeah, but if the server is never up, the array will keep on growing...
>>> 
>>> Gary
>>> 
>>>> On Wed, Aug 31, 2016 at 4:47 PM, Remko Popma <re...@gmail.com> wrote:
>>>> (Without looking at the code) I vaguely remember that class has a reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?
>>>> 
>>>> Sent from my iPhone
>>>> 
>>>>> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
>>>>> 
>>>>> We have this code in org.apache.logging.log4j.core.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String, FactoryData):
>>>>> 
>>>>>         @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, ex);
>>>>>                 return null;
>>>>>             }
>>>>>             try {
>>>>>                 // LOG4J2-1042
>>>>>                 final Socket socket = new Socket();
>>>>>                 socket.connect(new InetSocketAddress(data.host, data.port), data.connectTimeoutMillis);
>>>>>                 os = socket.getOutputStream();
>>>>>                 return new TcpSocketManager(name, os, socket, inetAddress, data.host, data.port,
>>>>>                         data.connectTimeoutMillis, data.delayMillis, data.immediateFail, data.layout);
>>>>>             } catch (final IOException ex) {
>>>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>>>>                 os = new ByteArrayOutputStream();
>>>>>             }
>>>>>             if (data.delayMillis == 0) {
>>>>>                 return null;
>>>>>             }
>>>>>             return new TcpSocketManager(name, os, null, inetAddress, data.host, data.port, data.connectTimeoutMillis,
>>>>>                     data.delayMillis, data.immediateFail, data.layout);
>>>>>         }
>>>>> 
>>>>> Notice:
>>>>> 
>>>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>>>>                 os = new ByteArrayOutputStream();
>>>>> 
>>>>> What is the point of that? It creates an unbound stream in RAM, creating one problem while dealing with another.
>>>>> 
>>>>> I think it would be better to not create the manager at all.
>>>>> 
>>>>> Thoughts?
>>>>> 
>>>>> Gary
>>>>> 
>>>>> -- 
>>>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
>>>>> Java Persistence with Hibernate, Second Edition
>>>>> JUnit in Action, Second Edition
>>>>> Spring Batch in Action
>>>>> Blog: http://garygregory.wordpress.com 
>>>>> Home: http://garygregory.com/
>>>>> Tweet! http://twitter.com/GaryGregory
>>> 
>>> 
>>> 
>>> -- 
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
>>> Java Persistence with Hibernate, Second Edition
>>> JUnit in Action, Second Edition
>>> Spring Batch in Action
>>> Blog: http://garygregory.wordpress.com 
>>> Home: http://garygregory.com/
>>> Tweet! http://twitter.com/GaryGregory
> 
> 
> 
> -- 
> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
> Java Persistence with Hibernate, Second Edition
> JUnit in Action, Second Edition
> Spring Batch in Action
> Blog: http://garygregory.wordpress.com 
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Gary Gregory <ga...@gmail.com>.
On Thu, Sep 1, 2016 at 11:40 PM, Remko Popma <re...@gmail.com> wrote:

> I just had a look at the code but I can't find a place where there's any
> attempt to get the bytes out of the ByteArrayOutputStream.
> If we're going to drop the messages anyway we should use a
> NullOutputStream.
>

So one fix would be for us to add and use a NullOutputStream class. The
other would be to flush the temp stream to the new stream on reconnect.

In one case, we loose data, in the other we potentially use a more RAM in
an unbounded way, if the server never is there for example.

There could be a third option where we use a bounded output stream.

What to do?

Gary

>
> Sent from my iPhone
>
> On 2016/09/02, at 12:46, Gary Gregory <ga...@gmail.com> wrote:
>
> Yeah, but if the server is never up, the array will keep on growing...
>
> Gary
>
> On Wed, Aug 31, 2016 at 4:47 PM, Remko Popma <re...@gmail.com>
> wrote:
>
>> (Without looking at the code) I vaguely remember that class has a
>> reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?
>>
>> Sent from my iPhone
>>
>> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
>>
>> We have this code in org.apache.logging.log4j.co
>> re.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String,
>> FactoryData):
>>
>>         @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, ex);
>>                 return null;
>>             }
>>             try {
>>                 // LOG4J2-1042
>>                 final Socket socket = new Socket();
>>                 socket.connect(new InetSocketAddress(data.host,
>> data.port), data.connectTimeoutMillis);
>>                 os = socket.getOutputStream();
>>                 return new TcpSocketManager(name, os, socket,
>> inetAddress, data.host, data.port,
>>                         data.connectTimeoutMillis, data.delayMillis,
>> data.immediateFail, data.layout);
>>             } catch (final IOException ex) {
>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>                 os = new ByteArrayOutputStream();
>>             }
>>             if (data.delayMillis == 0) {
>>                 return null;
>>             }
>>             return new TcpSocketManager(name, os, null, inetAddress,
>> data.host, data.port, data.connectTimeoutMillis,
>>                     data.delayMillis, data.immediateFail, data.layout);
>>         }
>>
>> Notice:
>>
>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>                 os = new ByteArrayOutputStream();
>>
>> What is the point of that? It creates an unbound stream in RAM, creating
>> one problem while dealing with another.
>>
>> I think it would be better to not create the manager at all.
>>
>> Thoughts?
>>
>> Gary
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> Java Persistence with Hibernate, Second Edition
>> <http://www.manning.com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition
> <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Remko Popma <re...@gmail.com>.
I just had a look at the code but I can't find a place where there's any attempt to get the bytes out of the ByteArrayOutputStream. 
If we're going to drop the messages anyway we should use a NullOutputStream. 

Sent from my iPhone

> On 2016/09/02, at 12:46, Gary Gregory <ga...@gmail.com> wrote:
> 
> Yeah, but if the server is never up, the array will keep on growing...
> 
> Gary
> 
>> On Wed, Aug 31, 2016 at 4:47 PM, Remko Popma <re...@gmail.com> wrote:
>> (Without looking at the code) I vaguely remember that class has a reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?
>> 
>> Sent from my iPhone
>> 
>>> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
>>> 
>>> We have this code in org.apache.logging.log4j.core.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String, FactoryData):
>>> 
>>>         @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, ex);
>>>                 return null;
>>>             }
>>>             try {
>>>                 // LOG4J2-1042
>>>                 final Socket socket = new Socket();
>>>                 socket.connect(new InetSocketAddress(data.host, data.port), data.connectTimeoutMillis);
>>>                 os = socket.getOutputStream();
>>>                 return new TcpSocketManager(name, os, socket, inetAddress, data.host, data.port,
>>>                         data.connectTimeoutMillis, data.delayMillis, data.immediateFail, data.layout);
>>>             } catch (final IOException ex) {
>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>>                 os = new ByteArrayOutputStream();
>>>             }
>>>             if (data.delayMillis == 0) {
>>>                 return null;
>>>             }
>>>             return new TcpSocketManager(name, os, null, inetAddress, data.host, data.port, data.connectTimeoutMillis,
>>>                     data.delayMillis, data.immediateFail, data.layout);
>>>         }
>>> 
>>> Notice:
>>> 
>>>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>>>                 os = new ByteArrayOutputStream();
>>> 
>>> What is the point of that? It creates an unbound stream in RAM, creating one problem while dealing with another.
>>> 
>>> I think it would be better to not create the manager at all.
>>> 
>>> Thoughts?
>>> 
>>> Gary
>>> 
>>> -- 
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
>>> Java Persistence with Hibernate, Second Edition
>>> JUnit in Action, Second Edition
>>> Spring Batch in Action
>>> Blog: http://garygregory.wordpress.com 
>>> Home: http://garygregory.com/
>>> Tweet! http://twitter.com/GaryGregory
> 
> 
> 
> -- 
> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
> Java Persistence with Hibernate, Second Edition
> JUnit in Action, Second Edition
> Spring Batch in Action
> Blog: http://garygregory.wordpress.com 
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Gary Gregory <ga...@gmail.com>.
Yeah, but if the server is never up, the array will keep on growing...

Gary

On Wed, Aug 31, 2016 at 4:47 PM, Remko Popma <re...@gmail.com> wrote:

> (Without looking at the code) I vaguely remember that class has a
> reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?
>
> Sent from my iPhone
>
> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
>
> We have this code in org.apache.logging.log4j.core.net.TcpSocketManager.
> TcpSocketManagerFactory.createManager(String, FactoryData):
>
>         @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,
> ex);
>                 return null;
>             }
>             try {
>                 // LOG4J2-1042
>                 final Socket socket = new Socket();
>                 socket.connect(new InetSocketAddress(data.host,
> data.port), data.connectTimeoutMillis);
>                 os = socket.getOutputStream();
>                 return new TcpSocketManager(name, os, socket, inetAddress,
> data.host, data.port,
>                         data.connectTimeoutMillis, data.delayMillis,
> data.immediateFail, data.layout);
>             } catch (final IOException ex) {
>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>                 os = new ByteArrayOutputStream();
>             }
>             if (data.delayMillis == 0) {
>                 return null;
>             }
>             return new TcpSocketManager(name, os, null, inetAddress,
> data.host, data.port, data.connectTimeoutMillis,
>                     data.delayMillis, data.immediateFail, data.layout);
>         }
>
> Notice:
>
>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>                 os = new ByteArrayOutputStream();
>
> What is the point of that? It creates an unbound stream in RAM, creating
> one problem while dealing with another.
>
> I think it would be better to not create the manager at all.
>
> Thoughts?
>
> Gary
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition
> <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Re: Memory leak in TCP appender in odd case

Posted by Remko Popma <re...@gmail.com>.
(Without looking at the code) I vaguely remember that class has a reconnect mechanism. Is the ByteArrayOutputStream used during the reconnect?

Sent from my iPhone

> On 2016/09/01, at 7:16, Gary Gregory <ga...@gmail.com> wrote:
> 
> We have this code in org.apache.logging.log4j.core.net.TcpSocketManager.TcpSocketManagerFactory.createManager(String, FactoryData):
> 
>         @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, ex);
>                 return null;
>             }
>             try {
>                 // LOG4J2-1042
>                 final Socket socket = new Socket();
>                 socket.connect(new InetSocketAddress(data.host, data.port), data.connectTimeoutMillis);
>                 os = socket.getOutputStream();
>                 return new TcpSocketManager(name, os, socket, inetAddress, data.host, data.port,
>                         data.connectTimeoutMillis, data.delayMillis, data.immediateFail, data.layout);
>             } catch (final IOException ex) {
>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>                 os = new ByteArrayOutputStream();
>             }
>             if (data.delayMillis == 0) {
>                 return null;
>             }
>             return new TcpSocketManager(name, os, null, inetAddress, data.host, data.port, data.connectTimeoutMillis,
>                     data.delayMillis, data.immediateFail, data.layout);
>         }
> 
> Notice:
> 
>                 LOGGER.error("TcpSocketManager (" + name + ") " + ex, ex);
>                 os = new ByteArrayOutputStream();
> 
> What is the point of that? It creates an unbound stream in RAM, creating one problem while dealing with another.
> 
> I think it would be better to not create the manager at all.
> 
> Thoughts?
> 
> Gary
> 
> -- 
> E-Mail: garydgregory@gmail.com | ggregory@apache.org 
> Java Persistence with Hibernate, Second Edition
> JUnit in Action, Second Edition
> Spring Batch in Action
> Blog: http://garygregory.wordpress.com 
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory