"Lock acquired but not safely unlocked" inspection

I believe it's safest to call lock() inside the try block, for the off chance
that lock() fails but leaves the lock in some half-locked state or something.

Andy DePue wrote:

However, isn't it best practice to write code like this?

 try { ... do something here ... } finally { lock.unlock(); } ]]>

Because we follow the above convention in our own code IDEA will flag our
code as violating the inspection - so, we usually end up disabling this
inspection. Remember that Lock is an interface, and even though it is highly
unlikely that the lock() method will fail (throw an exception), it is still
possible since you could be calling into any implementation. If one followed
what the intention wants, then there is the very small chance that you will
attempt to call unlock() on a Lock that was never locked to begin with, since
its lock() method failed to lock... if that makes sense. :) Should I open a
request in the tracker for this to be changed?

- Andy

ALSO, and far more importantly, an exception (like ThreadDeath) can be thrown at
any time, asynchronously, in the whitespace between your call to lock() and your
try block. In this case your lock would be left open.

Andy DePue wrote:

The "Lock acquired but not safely unlocked" inspection wants you to write code like this:

   try {
>     lock.lock();
>     ... do something here.
>   } finally {
>     lock.unlock();
>   }
> ]]>

However, isn't it best practice to write code like this?

   lock.lock();
>   try {
>     ... do something here ...
>   } finally {
>     lock.unlock();
>   }
> ]]>

Because we follow the above convention in our own code IDEA will flag our code as violating the inspection - so, we usually end up disabling this inspection. Remember that Lock is an interface, and even though it is highly unlikely that the lock() method will fail (throw an exception), it is still possible since you could be calling into any implementation. If one followed what the intention wants, then there is the very small chance that you will attempt to call unlock() on a Lock that was never locked to begin with, since its lock() method failed to lock... if that makes sense. :)
Should I open a request in the tracker for this to be changed?

- Andy

Yes, but what if Lock implementation of the unlock method throws an exception if lock is not acually acquired (as per JavaDoc comment of the unlock method?

It seems there is some problem there...

The various paired operation inspections are currently under consideration for whether they should allow the case where the start operation (lock(), in this case) occurs just before the try. I had originally thought they should be weakened for that case, but Keith's comment makes me think the current semantics may be correct, and putting the lock before the try is just dangerous enough to be worth triggering the inspection.

--Dave Griffith

I don't think there's a problem there. If lock() failed with an exception, then
unlock() can fail all it wants without causing semantics problems. Am I right?

Mileta Cekovic wrote:

Yes, but what if Lock implementation of the unlock method throws an exception if lock is not acually acquired (as per JavaDoc comment of the unlock method?

It seems there is some problem there...

> I believe it's safest to call lock() inside the try block, for the off chance
> that lock() fails but leaves the lock in some half-locked state or something.

Hm, I don't think so: If an exception is thrown by a supplier it is not the client's
responsibility to try and clean up the supplier's state.
More importantly, there are pragmatic arguments against it:

>> Yes, but what if Lock implementation of the unlock method throws an exception
>> if lock is not acually acquired (as per JavaDoc comment of the unlock method?
> I don't think there's a problem there. If lock() failed with an exception,
> then unlock() can fail all it wants without causing semantics problems. Am I right?
No, you're not ;)
"14.20.2 If the finally block completes abruptly for reason S, then the try statement
completes abruptly for reason S (and the throw of value V is discarded and forgotten)."
So an exception in the finally block will mask any exception from the try block -
so it would hide the real problem.

> ALSO, and far more importantly, an exception (like ThreadDeath) can be thrown
> at any time, asynchronously, in the whitespace between your call to lock() and
> your try block. In this case your lock would be left open.
ThreadDeath can only be thrown if someone called stop() which you should not really
use anyway (I think there's an inspection for that too).
Are there any other exception that might occur between a statement and the try block?

I completely agree with Stephen!

The only valid argument for putting lock() inside try block I read in this discussion is that ThreadDeath exception can occur after successfull lock() invocation and before try/finally exception handler is formed. But Stephen arguments eliminate almost all my doubts. Is there still more arguments for using lock() inside try block?

Stephen Kelvin wrote:

> I believe it's safest to call lock() inside the try block, for the
off chance
> that lock() fails but leaves the lock in some half-locked state or
something.

Hm, I don't think so: If an exception is thrown by a supplier it is not
the client's
responsibility to try and clean up the supplier's state.
More importantly, there are pragmatic arguments against it:

>> Yes, but what if Lock implementation of the unlock method throws an
exception
>> if lock is not acually acquired (as per JavaDoc comment of the
unlock method?
> I don't think there's a problem there. If lock() failed with an
exception,
> then unlock() can fail all it wants without causing semantics
problems. Am I right?
No, you're not ;)
"14.20.2 If the finally block completes abruptly for reason S, then the
try statement
completes abruptly for reason S (and the throw of value V is discarded
and forgotten)."
So an exception in the finally block will mask any exception from the
try block -
so it would hide the real problem.

Okay, so unlock() should go in its own try block, right?

> ALSO, and far more importantly, an exception (like ThreadDeath) can
be thrown
> at any time, asynchronously, in the whitespace between your call to
lock() and
> your try block. In this case your lock would be left open.
ThreadDeath can only be thrown if someone called stop() which you should
not really
use anyway (I think there's an inspection for that too).
Are there any other exception that might occur between a statement and
the try block?

Any exception can be thrown by the JVM asynchronously. I don't know which ones
it actually throws. Anyway, you're going to risk your application's semantics
assuming no one will call stop(), and that you know everything about the VM's
asynchronous exception usage?

Keith Lea wrote:

Any exception can be thrown by the JVM asynchronously. I don't know
which ones it actually throws. Anyway, you're going to risk your
application's semantics assuming no one will call stop(), and that you
know everything about the VM's asynchronous exception usage?

The VM allows any exception to be thrown asynchronously via JVMTI. You can see
http://java.sun.co%5cm/j2se/1.5.0/docs/guide/jvmti/jvmti.html#StopThread for
details.

Are there any other exception that might occur between a statement and the try block?

Per the JLS, the only ways to cause an asynchronous exception are stop() and an internal JVM error.

I'm still not taking sides on this issue, but the possibility of asychronous exceptions is still striking me as a strong argument for requiring the lock() to be inside the try(). The fact that the unlock() will need it's own try block is unfortunate but not enough to change the analysis.

--Dave Griffith

Serves me right for going by JLS2.0. In 1.5, you can absolutely cause an asynchronous exception to be thrown via the JVM debugging interface. I actually knew that, but forgot. Doesn't really effect the argument, as you can do so much horrible stuff via the JVMTI that there's no way inspections should be aware of those possibilities.

--Dave Griffith

Thanks for the info! I've been assuming that if a lock method throws an exception then it didn't get a lock - so it's good to get that assumption taken care of. Guess we'll be changing our convention. :)

- Andy

Andy DePue wrote:

Thanks for the info! I've been assuming that if a lock method throws an exception then it didn't get a lock - so it's good to get that assumption taken care of. Guess we'll be changing our convention. :)

I'd still assume it to be that way. I consider "taking a lock" like "invoking a
constructor": If it fails, you don't get a new object. I believe this to be the same with
locks. In any case you're dependent on a correct implementation of those things, otherwise
there's always the chance to end up in a big mess, non matter what precautions you take.

I prefer to have such statements before the try-block.

Sascha

Sascha Weireuter wrote:

I prefer to have such statements before the
try-block.

Me too, and since I'm the mainainter for now an option probably will go in:-) Same for all the other "resource" inspections. If somebody could submit a JIRA issue for this, what would be great.

Bas

Already exists. http://jetbrains.net/jira/browse/IDEABKL-3304.

I've just reassigned it to you.

(Wierd seeing the IJ bug after your name, but cool...)

--Dave Griffith

Feels wierd to see my name there too. I feel like a kid whose has just been handed the keys to the toy store.

Bas

By the way, I got my Sun TechTip today which talked about just this
topic - they recommend locking outside the try block.
I checked with my copy of Concurrent Programming, DL recommends the same
technique, i.e.

not

Just another datapoint for the debate!
R