CODEWITHSUNDEEP
javamultithreadingsonarqube
Hai Hoang
Hai Hoang

Reputation: 1257

How to fix Sonar issue "Remove this call to "wait" or move it into a "while" loop"?

I am received a request to fix sonar issues in a legacy project, there is a code segment like this, every call to this function will be paused for 50ms:

synchronized(monitor) {

  [...]

  try {

    [...]

    Thread.sleep(config.getWaitTime()); // return 50
  } catch (SomeException e) {
    log.error(e.getMessage(), e);
  }

  [...]
}

First, sonar asked me to change Thread.sleep() to wait() so I change the try block to to:

try {

  [..]

  monitor.wait(config.getWaitTime());
} catch (SomeException e) {
  log.error(e.getMessage(), e);
}

Then, another issue appear : Remove this call to "wait" or move it into a "while" loop , I do not have much experience with multithreading, so I'm not sure my fix is correct:

boolean wait = true;
while (wait) {
  wait = false;
  monitor.wait(config.getWaitTime());
}

Is the above solution correct? If not, what should I do?

Upvotes: 2

Views: 3721

Answers (3)

talex
talex

Reputation: 20543

sonar asked me to change Thread.sleep() to wait()

Just ignore sonar here. It is false positive alarm.

Upvotes: 1

davidxxx
davidxxx

Reputation: 131496

Here the while appears helpless :

boolean wait = true;
while (wait) {
  wait = false;
  monitor.wait(config.getWaitTime());
}

The while statement around the wait() statement is designed to check that the wait() be re-invoked if the logical/function condition in the while is true but in your case it is never as you assign wait to false as first statement of the while body.
The while is so helpless.
Here is the Object.wait(long) javadoc associated to :

A thread can also wake up without being notified, interrupted, or timing out, a so-called spurious wakeup. While this will rarely occur in practice, applications must guard against it by testing for the condition that should have caused the thread to be awakened, and continuing to wait if the condition is not satisfied.

As often I am not convinced at all of Sonar advises....
You could use a timer to check that the required time was elapsed in the while. But ugg.
I advise you to keep the Thread way that suits to your requirement.
Or if you want to make the tool API add helpless code. Of course I am joking : don't do that !

Upvotes: 2

user10639668
user10639668

Reputation:

From the Object#wait() Java doc

A thread can also wake up without being notified, interrupted, or timing out, a so-called spurious wakeup. While this will rarely occur in practice, applications must guard against it by testing for the condition that should have caused the thread to be awakened, and continuing to wait if the condition is not satisfied. In other words, waits should always occur in loops, like this one:

 synchronized (obj) {
     while (<condition does not hold>)
         obj.wait(timeout);
     ... // Perform action appropriate to condition
 }

Your loop doesn't look good, it should be something like:

while (wait) {
  monitor.wait(config.getWaitTime());
}

The wait variable must be set from elsewhere, when the wait condition is no longer needed.

Upvotes: 2

Related Questions