False positive when value set in try-block squid:S2583

[rus...@gmail.com]

I'm trying to implement the safe locking idiom described here: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html

But for a conditional lock

The following code triggers squid:S2583

boolean hasLock = false;

try {

hasLock = lock.tryLock(LOCK_WAIT, TimeUnit.MILLISECONDS);

if (hasLock) {

/* Do stuff */

}

} catch (InterruptedException ex) {

throw new LockingException(LOCK_RESET_MSG + this.statID, ex);

} finally {

if (hasLock) {

consumerLock.unlock();

}

}

This triggers squid:S2583 " Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"" though obviously if the lock is acquired hasLock will be true, or false otherwise.

Could it be the case that it is ignoring the set of the local variable within the try scope? That the value will be set or not "may" be ambiguous but it would not be certainly one value or the other ...

[Francis Galiegue]

On Friday, April 1, 2016 at 4:19:46 PM UTC+2, Robert wrote:

I'm trying to implement the safe locking idiom described here: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html

But for a conditional lock

The following code triggers squid:S2583

boolean hasLock = false;

try {

hasLock = lock.tryLock(LOCK_WAIT, TimeUnit.MILLISECONDS);

if (hasLock) {

/* Do stuff */

}

} catch (InterruptedException ex) {

throw new LockingException(LOCK_RESET_MSG + this.statID, ex);

} finally {

if (hasLock) {

consumerLock.unlock();

}

}

Hello,

Regardless of the false positive, this is not how you should lock anyway. The recommended idiom is as follows:

----

try {

    if (lock.tryLock(...)) {

        try {

            // do something

        } finally {

            lock.unlock();

        }

    } else {

        // lock not acquired

    }

} catch (InterruptedException e) {

    // deal with InterruptedException

}

----

In your original code, consider what would happen if _another_ statement in "if (hashLock)" could throw an InterruptedException: you would end up incorrectly throwing a LockingException.

Regards, 

[Robert Rusk]

Thanks very much for the feedback. I actually had the code as you described originally, but that was throwing "other" sonar false positives (squid:S2222 "Locks should be released" I think) because the rule doesn't recognise either the 'tryLock' as an acquisition or else the 'lock.unlock' in the finally block ...

In the code example which I believe the LockingException is in fact expected to be thrown elsewhere, so it is a bad example in a way, but I'll take a closer look at where else I've applied this idiom to be sure ...

Thanks again!

Rob

--
You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/e8eC9BkJQDc/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/5d97a21d-cfee-4b15-ad4d-502448839b67%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Nicolas Peru]

Hi Robert, 

Could you please precise which version of the java plugin you are using ? 

For me this is a flaw in the CFG that should be fixed in the latest release of the java plugin. Could you give a try with version 3.13 and tell us if the issue persists ?

Thanks

Cheers, 

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/CADpExxt%2BATeXcOxvPtW8Q%2BMQyzKmgdP7R6HA0tfKe0Wnp-hnHA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Julien Herr]

Hi,

I up the post because I think I have a similar issue.

You can have a look on it here: https://sonarqube.com/issues/search#issues=AVaKuz-u6a8oOfyElMRZ

Regards,

Julien

[Julien Herr]

Another example: https://sonarqube.com/issues/search#issues=AVaKu0NU6a8oOfyElMRg

[Nicolas Peru]

Hi Julien, 

This second example is not a FP imo : we consider method parameters should not be null, and this is the only way where this condition could be reach with a null value.

I'm looking at the first issue, but next time please don't hijack threads and open a new one. This is way easier to monitor. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/d6658e8d-5637-4254-9002-28c492f50e11%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Julien Herr]

Le mercredi 31 août 2016 11:30:08 UTC+2, Nicolas Peru a écrit :

Hi Julien, 

This second example is not a FP imo : we consider method parameters should not be null, and this is the only way where this condition could be reach with a null value.

Oh, really? It's a bit optimistic to expect not null parameters.

What if I add @Nullable on the parameter?

 

I'm looking at the first issue, but next time please don't hijack threads and open a new one. This is way easier to monitor. 

Ok, sorry! I supposed this thread is similar to my "issues".

[Nicolas Peru]

What if I add @Nullable on the parameter?

Well, that's another story... And in this case we explore both cases (null or not). 

CHeers,

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/d2e5a2a9-ef0b-44c0-baea-494478aed9ab%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Nicolas Peru]

Hi Julien, 

Can you open a separate thread with your first case inlined (for sake of history) ? I have a clearer idea of what is going on and it has nothing to do with this thread. Thanks. 

Cheers,