Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

findbug issues to consider fixing in spec java code #188

Closed
jfialli opened this Issue Jul 17, 2013 · 2 comments

Comments

Projects
None yet
2 participants

jfialli commented Jul 17, 2013

Default Level Warning:

Wa: Wait not in loop (WA_NOT_IN_LOOP)

This method contains a call to java.lang.Object.wait() which is not in a loop. If the monitor is used for multiple conditions, the condition the caller intended to wait for might not be the one that actually occurred.

File: CompletionListenerFuture.java

@override
public Void get() throws InterruptedException, ExecutionException {
synchronized (this) {
if (!isCompleted) { //fix is to change this to "while (!isCompleted) {
// this bug can not happen way code is written.
wait();
}

  if (exception == null) {
    return null;
  } else {
    throw new ExecutionException(exception);
  }
}

}


Low Level Warning:

No: Using notify() rather than notifyAll() (NO_NOTIFY_NOT_NOTIFYALL)

This method calls notify() rather than notifyAll(). Java monitors are often used for multiple conditions. Calling notify() only wakes up one thread, meaning that the thread woken up might not be the one waiting for the condition that the caller just satisfied.

Note that this findbug can not happen with way code is written.
Option is to exclude this warning or just fix to please findbugs.

 / **
  • Notifies the application that the operation completed successfully.

  • @throws IllegalStateException if the instance is used more than once
    */
    @override
    public void onCompletion() throws IllegalStateException {
    synchronized (this) {
    if (isCompleted) {
    throw new IllegalStateException("Attempted to use a CompletionListenerFuture instance more than once");
    } else {
    isCompleted = true;
    notify();
    }
    }
    }

    /**

  • Notifies the application that the operation failed.
    *

  • @Param e the Exception that occurred

  • @throws IllegalStateException if the instance is used more than once
    */
    @override
    public void onException(Exception e) throws IllegalStateException {
    synchronized (this) {
    if (isCompleted) {
    throw new IllegalStateException("Attempted to use a CompletionListenerFuture instance more than once");
    } else {
    isCompleted = true;
    exception = e;
    notify();
    }
    }
    }

Owner

gregrluck commented Jul 18, 2013

I have added the findbugs plugin, which was quite a pain with the changes to mvn 3 and the way it operates through reports.

Will fine tune the report tomorrow so that we generate it on demand and for when we submit the final draft.

@ghost ghost assigned gregrluck Jul 19, 2013

Owner

gregrluck commented Jul 19, 2013

Got findbugs working with mvn compile site. It generates an HTML report in the target/site.

Fixed WA_NOT_IN_LOOP as suggested.

Excluded the rest with a findbugs-exclude.xml which is added to the build.

@gregrluck gregrluck closed this Jul 19, 2013

@gregrluck gregrluck added a commit that referenced this issue Jul 19, 2013

@gregrluck gregrluck Fixed #188. Made one change as recommended by findbugs and excluded t…
…hree false positives.
6457eb0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment