JRuby

FindBugs issues

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: JRuby 1.1RC3
  • Component/s: None
  • Labels:
    None
  • Patch Submitted:
    Yes
  • Number of attachments :
    2

Description

I ran FindBugs 1.2.1 over JRuby trunk (the Eclipse plugin is quite nice).

Some issues fixed by the attached patch:

Various minor issues.

Inconsistent synchronization of org.jruby.RubyThread.finalResult
Unsynchronized access at RubyThread.java:[line 245]
Synchronized access at RubyThread.java:[line 401]

RubyString.java:[line 1732]
Bad attempt to compute absolute value of signed 32-bit random integer
This code generates a random signed integer and then computes the absolute value of that random integer. If the number returned by the random number generator is Integer.MIN_VALUE, then the result will be negative as well (since Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE).

Issues that look like they should be fixed:

org.jruby.RubyTime uses a static DateFormat, as does org.jvyamlb.RepresenterImpl$DateYAMLNodeCreator. This might be a problem because "as the JavaDoc states, DateFormats are inherently unsafe for multithreaded use."

In RubyModule's addClassProvider:
This method may contain an instance of double-checked locking. This idiom is not correct according to the semantics of the Java memory model. For more information, see the web page
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

In RubyZlib.java:[line 784] ce is compared to -1:
Bad comparison of nonnegative value with negative constant
This code compares a value that is guaranteed to be non-negative with a negative constant.

There are various cases reported as "Inefficient use of keySet iterator instead of entrySet iterator".

Attachments

  1. File
    bench_var.rb
    28/Jun/07 9:02 PM
    1 kB
    Bill Dortch
  2. Text File
    findbugs.patch
    27/Jun/07 10:12 PM
    16 kB
    Albert Strasheim

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Bill Dortch added a comment -

Re:

In RubyModule's addClassProvider:
This method may contain an instance of double-checked locking. This idiom is not correct...

I'm not worried right now about this particular usage, since, as things currently stand, a) there will be at most one ClassProvider for a module, and b) re-initialization would be harmless, as there's only one instance of ClassProvider, period. (Some may recall this was part of my little hack to enable the double-colon syntax for opening Java classes - I sort of over-engineered this bit )

However, there is a more worrisome instance of this usage in RubyObject#getInstanceVariables. Unless someone has a better idea, I'm going to just make that a synchronized method.

Show
Bill Dortch added a comment - Re:
In RubyModule's addClassProvider: This method may contain an instance of double-checked locking. This idiom is not correct...
I'm not worried right now about this particular usage, since, as things currently stand, a) there will be at most one ClassProvider for a module, and b) re-initialization would be harmless, as there's only one instance of ClassProvider, period. (Some may recall this was part of my little hack to enable the double-colon syntax for opening Java classes - I sort of over-engineered this bit ) However, there is a more worrisome instance of this usage in RubyObject#getInstanceVariables. Unless someone has a better idea, I'm going to just make that a synchronized method.
Hide
Permalink
Andrew Geweke added a comment -

FWIW, the issues with double-checked locking aren't just that you can get a double initialization – you can actually get back a broken object. See: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Basically, double-checked locking is completely broken, plain and simple, and instances where it exists can cause essentially arbitrary broken behavior (use of created-but-uninitialized (constructor not run, or run incompletely) objects, among other things). It's worth fixing. I'll try to upload a patch if I get a chance shortly – alas have been quite busy recently...

Show
Andrew Geweke added a comment - FWIW, the issues with double-checked locking aren't just that you can get a double initialization – you can actually get back a broken object. See: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html Basically, double-checked locking is completely broken, plain and simple, and instances where it exists can cause essentially arbitrary broken behavior (use of created-but-uninitialized (constructor not run, or run incompletely) objects, among other things). It's worth fixing. I'll try to upload a patch if I get a chance shortly – alas have been quite busy recently...
Hide
Permalink
Bill Dortch added a comment -

Changed RubyModule#addClassProvider to synchronized method (in 3919).

I reviewed the patch – it's straightforward and looks fine, but someone more familiar with the compiler/lexer/etc. sections of the codebase should probably double-check (no pun intended) before applying.

I've held off on the more critical (I believe) patch to remove the double-check from RubyObject#getInstanceVariables, in the hope that someone can suggest an alternative to synchronizing the method. Doing so, i.e.

public synchronized Map getInstanceVariables() {
        if (instanceVariables == null) {
            instanceVariables = Collections.synchronizedMap(new HashMap());
        }
        return instanceVariables;
    }

results in about a 5% degradation in instance variable access speed, as measured by the attached benchmark (bench_var.rb). I looked at the possibility of two methods, with an unsynchronized version that would call the synchronized one; but as I read the DoubleCheckedLocking document, we'd end up with the same problem. True?

I should note that overall run times of ant test-interpreted and test-compiled were essentially unaffected by the change (they actually ran a tiny bit faster with the change in a couple of trials). Not that that's a real benchmark, but it might help put the impact of this change in context.

Show
Bill Dortch added a comment - Changed RubyModule#addClassProvider to synchronized method (in 3919). I reviewed the patch – it's straightforward and looks fine, but someone more familiar with the compiler/lexer/etc. sections of the codebase should probably double-check (no pun intended) before applying. I've held off on the more critical (I believe) patch to remove the double-check from RubyObject#getInstanceVariables, in the hope that someone can suggest an alternative to synchronizing the method. Doing so, i.e. 
public synchronized Map getInstanceVariables() {
        if (instanceVariables == null) {
            instanceVariables = Collections.synchronizedMap(new HashMap());
        }
        return instanceVariables;
    }
results in about a 5% degradation in instance variable access speed, as measured by the attached benchmark (bench_var.rb). I looked at the possibility of two methods, with an unsynchronized version that would call the synchronized one; but as I read the DoubleCheckedLocking document, we'd end up with the same problem. True? I should note that overall run times of ant test-interpreted and test-compiled were essentially unaffected by the change (they actually ran a tiny bit faster with the change in a couple of trials). Not that that's a real benchmark, but it might help put the impact of this change in context.
Hide
Permalink
Bill Dortch added a comment -

Third time's the charm...

Show
Bill Dortch added a comment - Third time's the charm...
Hide
Permalink
Charles Oliver Nutter added a comment -

We should tidy whatever's left for 1.1 release. I don't think they'll be difficult. Maybe even fun!!!

Show
Charles Oliver Nutter added a comment - We should tidy whatever's left for 1.1 release. I don't think they'll be difficult. Maybe even fun!!!
Hide
Permalink
Charles Oliver Nutter added a comment -

Punting issues from 1.1 RC2 to 1.1 final.

Show
Charles Oliver Nutter added a comment - Punting issues from 1.1 RC2 to 1.1 final.
Hide
Permalink
Charles Oliver Nutter added a comment -

I made all appropriate changes from this patch in r6246. Thanks!

Show
Charles Oliver Nutter added a comment - I made all appropriate changes from this patch in r6246. Thanks!
Hide
Permalink
Albert Strasheim added a comment -

FindBugs 1.3.2 still finds quite a few serious issues in the latest JRuby trunk. One of the main developers probably needs to run it and take a look.

Show
Albert Strasheim added a comment - FindBugs 1.3.2 still finds quite a few serious issues in the latest JRuby trunk. One of the main developers probably needs to run it and take a look.
Hide
Permalink
Thomas E Enebo added a comment -

Resolving based on this attachment for this issue. If you generate a new one then open a new issue to track that.

Show
Thomas E Enebo added a comment - Resolving based on this attachment for this issue. If you generate a new one then open a new issue to track that.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.