0

Relatively new to Java but tasked with looking at Coverity defects. Is this one as simple as I think? (I can't convince the code owner.)

 50       // this method is used when the program is run as a pojo.
 51        public static void main(final String[] args) {
 52
 53                startProgram();
 54
 55                // keep this here, for console app, needs to create a loop, but not for AS apps.
 56                lock = new Object();
 57

loop_outside_lock_insufficient: Because this loop occurs outside the locked region, it cannot be a sufficient check of the wait condition. The value of the wait condition may change between the check of the loop condition and the call to wait.
 58                while (!finished) {

lock_acquire: Acquiring lock program.lock.
 59                        synchronized (lock) {
 60                                try {

CID 711645 (#1 of 1): Indefinite wait (BAD_CHECK_OF_WAIT_COND)wait_cond_improperly_checked: The wait condition prompting the wait upon program.lock is not checked correctly. This code can wait for a condition that has already been satisfied, which can cause a never-ending wait.

Refactor the code to protect the call to wait with a loop that rechecks the wait condition inside the locked region.
 61                                        lock.wait();
 62                                } catch (final InterruptedException e) {
 63
 64                                        log.error(e.getStackTrace());
 65                                }
 66                        }
 67                }
 68
 69        }

I think I just want to change this to 

synchronized(lock)
    while(!finished){
        try lock.wait();
        catch(InterruptedException e)
            log.error(e.getStackTrace());
        }

Does that make sense? Do I really just need to move the while loop inside the lock statement?

Sinc
  • 553
  • 1
  • 8
  • 31

1 Answers1

2

You need to declare finished as volatile. Then your solution is okay.

cruftex
  • 5,545
  • 2
  • 20
  • 36
  • I had to lookup volatile. Found good information at http://stackoverflow.com/questions/106591/do-you-ever-use-the-volatile-keyword-in-java. If there is only one method that sets `finished` I should still use the `volatile` flag, but can I actually get rid of `synchronized`? – Sinc Mar 11 '16 at 20:12
  • No, that's needed for wait(). – cruftex Mar 11 '16 at 20:14
  • Moving the code and adding volatile worked for me too but I don't fully understand why Coverity complained in the first place. Even after reading the reason why, it is still unclear as to the issue. – Unhandled Exception Jun 18 '19 at 13:14