[logback-dev] IllegalMonitorStateException inAppenderAttachableImpl.appendLoopOnAppenders()

[Joern Huxhorn]

Hi Ceki,

On 03.02.2009, at 17:54, Ceki Gulcu wrote:

>
>
> Hi Maarteen,
>
> Maarten Bosteels wrote:
>> Hello,
>> I think moving r.lock(); outside of the try block WILL help,  
>> because the original exception will no longer be hidden.
>

Even though I hate being a smart ass and I also don't like to say  
something like "See, I said so", I have to do just that.

This is exactly what I meant when I wrote
"It's generally a very bad idea to save a bit of standard code just  
because a possible problem isn't imaginable at the moment. I'm not  
even sure if the code without try {} finally {} will execute faster or  
if there won't be any difference at all. "
in http://jira.qos.ch/browse/LBCORE-67 and is also the prime reason  
why I submitted the ticket in the first place.

The documentation of the Lock interface at http://java.sun.com/javase/6/docs/api/java/util/concurrent/locks/Lock.html 
  clearly states:
<quote>
With this increased flexibility comes additional responsibility. The  
absence of block-structured locking removes the automatic release of  
locks that occurs with synchronized  methods and statements. In most  
cases, the following idiom should be used:

Lock l = ...;
l.lock();
try {
         // access the resource protected by this lock
} finally {
         l.unlock();
}
</quote>
The "should" is only there because there might be an obscure case  
where a different pattern would probably be helpful - and this isn't  
such a case.

> Moving r.lock() outside the try block MAY help because the finally
> block which throws a related but distinct exception will no longer be
> invoked. The second exception effectively hides the first
> exception. However, this is true if r.lock() throws an exception
> *visible* in logback code. I tend to think, for reasons explained in
> my previous post, that somewhere within r.lock() call there is a
> RuntimeException thrown but which is absorbed somewhere within the
> r.lock() invocation. So, there is a good chance (but less than 100%)
> that displacing the r.lock() call to outside the try block will *not*
> reveal anything new.

The documentation of the lock() method at http://java.sun.com/javase/6/docs/api/java/util/concurrent/locks/Lock.html#lock() 
  states:
<quote>
Implementation Considerations

A Lock implementation may be able to detect erroneous use of the lock,  
such as an invocation that would cause deadlock, and may throw an  
(unchecked) exception in such circumstances. The circumstances and the  
exception type must be documented by that Lock implementation.
</quote>

Furthermore, the documentation of ReentrantReadWriteLock (the one used  
in AppenderAttachableImpl) at http://java.sun.com/javase/6/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html 
  does actually document a case where exactly such a scenario happens:
<quote>
Implementation Notes

This lock supports a maximum of 65535 recursive write locks and 65535  
read locks. Attempts to exceed these limits result in Error throws  
from locking methods.
</quote>

While this most likely isn't the case here it certainly illustrates  
one thing:
Just stick to the pattern.

> Having said that, if for one reason or another, the r.lock() call
> throws an exception, which it certainly does, even if it is not
> visible, there is arguably no point in calling r.unlock(). Calling
> r.lock() outside the try block MAY reveal the original exception
> without having a downside.

It will.

> On the other hand, if calling r.unlock has
> a positive side-effect even if r.lock() fails, then moving r.lock()
> outside the try block may not be such a good idea after all.

It won't have a positive effect because it would be - in fact - an  
undocumented behavior and that is *never* something to desire.

lock() does only throw an unchecked exception if it couldn't acquire  
the lock (for one reason or another) while unlock() throws the  
exception mentioned in the subject if the lock is not held (see http://java.sun.com/javase/6/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.WriteLock.html#unlock() 
  ).

> To summarize, although not guaranteed, there is a good chance that
> moving the r.lock() call outside the try block will reveal new
> information. However, this change may be problematic in certain cases,
> which at this time we can only imagine but not pinpoint to. In
> Rumsfeld-speak, moving r.lock has a known but uncertain positive
> effect and uncertain and unknown negative effects.
>

Everything is perfectly documented and defined. This is mainly caused  
by the fact that java.util.concurrent was created by Doug Lea and this  
guy really knows what he's doing. His book on concurrency is quite  
excellent.
Beside that, I dislike Rumsfeld ;)

> Alternatively, we could invoke r.unlock in a safe manner, that is
> within a try/catch block (embedded within finally). This would not
> obfuscate the first exception and would ensure that r.unlock is always
> invoked.
>
> I am not sure splitting hairs as demonstrated above gets us any closer
> to a solution. Zoltan, do you need any specific help from my side?

I fail to see the hairsplitting in Maartens message and I agree with  
him.

Instead of invoking unlock and circumventing the built in security  
measures using try{...}catch(Throwable){} we should just use the Lock  
as defined in the API.

Sorry for being nosy,
Joern.