[PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Joern Huxhorn <jo...@gmx.de>]

Hi all.

I hope I submit this patch the right way - never did this before and 
tried really hard ;)

The attached patch fixes the problems with the ObjectOutputStream-memory 
leak. It does also reduce the amount of lost events if the app is 
terminated without calling LogManager.shutdown() first.

Greetings,
Joern.

Re: [PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Joern Huxhorn <jo...@gmx.de>]

Hi again.

I forgot to mention that it's a good idea to reset the 
ObjectOutputStream before flushing because oos.reset() writes a token to 
the stream. I took that into account in my change.

Yours, Joern.


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: log4j-dev-help@jakarta.apache.org

Re: [PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Joern Huxhorn <jo...@gmx.de>]

Hi Ray.

> Also, IMHO, Joern was trying to introduce more buffering to the 
> SocketAppender.  However, I think that a better way to implement the 
> buffering would be to chain the stream with a BufferedOutputStream or 
> to use Socket.setSendBufferSize(). 

You're right, that's exactly what I'm trying. I did, in fact, chain the 
stream with a BufferedOutputStream (bos), too. The ByteArrayOutputStream 
is in there because I first tried to
close the ObjectOutputStream to get rid of it completely which did not 
work because the ObjectInputStream would state that the stream is 
corrupted - this was generally a bad idea (my first mail, please ignore, 
I was very sleepy ;)) because it would also recreate the oos all the time.

It may be possible that the baos isn't strictly necessary but at least 
it ensures that the LoggingEvent can be serialized very fast (without 
any blocking due to buffers) which wouldn't be guaranteed if I'd just 
use a BufferedOutputStream. The buffer could be full in the middle of an 
Object and therefore block.

This code is just an idea to work around the Windows-bug mentioned in 
the javadoc. Calling LogManager.shutdown() isn't always possible (e.g in 
case of a crash where you're especially interested in the last few 
LoggingEvents or if Logging is done by means of Commons-Logging without 
hard-wiring Log4J at all) so I just tried to reduce the loss of events 
by reducing the possible blocks.

I don't think that there are any threading issues but I'm not 100% sure 
because I'm not familiar with the surrounding architecture. I think 
there are none since all I did was synchronizing all methods of 
SocketAppender that use the attribute bos. This is necessary since bos 
could be null/change and therefore isn't available for synchronization 
itself.

It sounds quite sane to me that the SocketAppender can not connect/close 
while writing an event and vice versa. That's also the reason why I 
changed line 363 which was previously synchronizing on the 
Connector-Thread instead of the SocketAppender.

Yours, Joern.



---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: log4j-dev-help@jakarta.apache.org

Re: [PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Raymond DeCampo <rd...@twcny.rr.com>]

Ceki Gülcü wrote:
> 
> Hi Joern,
> 
> Which ObjectOutputStream-memory leak are you referring to?
> 
> I have also looked at your next patch. I fail to understand your 
> objectives. What is the goal?
> 

Hello Ceki,

I was looking at this code and it occurred to me that introducing a 
buffer like this would introduce some threading issues.  Then it 
occurred to me that there might be threading issues in the original 
code.  For example, I don't see how two threads might be prevented from 
calling oos.writeObject() at the same time.  Finally it occurred to me 
that it might just be that I do not see it. :)

Anyway, is there really a threading issue or is it taken care of in some 
other way?

Also, IMHO, Joern was trying to introduce more buffering to the 
SocketAppender.  However, I think that a better way to implement the 
buffering would be to chain the stream with a BufferedOutputStream or to 
use Socket.setSendBufferSize().

Thanks,
Ray


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: log4j-dev-help@jakarta.apache.org

Re: [PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Joern Huxhorn <jo...@gmx.de>]

Hi Ceki.

The memory-leak isn't there in the current implementation. You added the 
oos.reset() and wrote in the comments that it's there because a 
memory-leak would occurr if it's removed. Please ignore my first mail 
completely - I made a stupid mistake.

The objective of my second patch is to prevent blocking of the sockets 
stream by using buffers which helped reducing the data-loss-problem 
described in the javadoc of SocketAppender.
It does exactly that on my setup and shouldn't impose any problems (in 
contrast to my first patch :( ).

I seldom lose any events at all anymore even in the rather unfair 
attached Foo-class. Well, at least if Chainsaw is the active application 
which could also be fixed as described in my previous mail by changing 
the priority of chainsaw's receiver-thread to NORM_PRIORITY+1. This 
would help if Chainsaw and the logged application are running on the 
same machine (and the application uses threads with 
priority<=NORM_PRIORITY - otherwise it's just not resonable to run 
Chainsaw on the same machine...).

I think the added synchronized's are necessary and of no harm to the 
rest of the system while the synchronized(this) in line 363 of the 
original code should really be synchronized(SocketAppender.this) instead 
because it synchronizes access to the sockets stream managed by 
SocketAppender.this.
So these changes and the LogLog-changes are meant to be fixes. I think 
the new LogLog-behaviour fixes bug 17331 because the casual user wants 
the error-message but isn't interested in the stack-trace at all while 
the developer gets the trace if he needs it. My boss hates the current 
behaviour because it looks like an error to the end-user of the app ;)

I hope this claryfies my point a bit.
And while I have the chance: Thank you sooooo much for Log4J!

J�rn.

Ceki G�lc� wrote:

>
> Hi Joern,
>
> Which ObjectOutputStream-memory leak are you referring to?
>
> I have also looked at your next patch. I fail to understand your 
> objectives. What is the goal?
>
> At 11:43 AM 5/2/2003 +0200, you wrote:
>
>> Hi all.
>>
>> I hope I submit this patch the right way - never did this before and 
>> tried really hard ;)
>>
>> The attached patch fixes the problems with the 
>> ObjectOutputStream-memory leak. It does also reduce the amount of 
>> lost events if the app is terminated without calling 
>> LogManager.shutdown() first.
>>
>> Greetings,
>> Joern.
>

Re: [PATCH] SocketAppender-patch for the ObjectOutputStream.reset()-bug.

[Ceki Gülcü <ce...@qos.ch>]

Hi Joern,

Which ObjectOutputStream-memory leak are you referring to?

I have also looked at your next patch. I fail to understand your 
objectives. What is the goal?

At 11:43 AM 5/2/2003 +0200, you wrote:
>Hi all.
>
>I hope I submit this patch the right way - never did this before and tried 
>really hard ;)
>
>The attached patch fixes the problems with the ObjectOutputStream-memory 
>leak. It does also reduce the amount of lost events if the app is 
>terminated without calling LogManager.shutdown() first.
>
>Greetings,
>Joern.

--
Ceki  For log4j documentation consider "The complete log4j manual"
       http://www.qos.ch/shop/products/clm_t.jsp 


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: log4j-dev-help@jakarta.apache.org