4 ms·

I once caused a serious, halt-the-enterprise production bug by "fixing" a problem found by FindBugs. This was Java code, something along the lines of: Boolea

I once caused a serious, halt-the-enterprise production bug by "fixing" a problem found by FindBugs. This was Java code, something along the lines of: Boolean b = new Boolean(true); The static analyzer correctly identified this as an unnecessary new object creation (style guides and good sense recommend you simply use Boolean.TRUE). I "fixed" it, and went on my way. Little did I realize that this variable was actually a lock, and there was a synchronized(b) block later (and much deeper) in the code, which I effectively eliminated by removing the new. In my defense I feel that the real bug here was that of documentation- had the variable been named something like "lock" I'd have understood immediately what was going on. But that doesn't make you feel much better when your team's been up all night fixing your bug! Moral of the story: your codebase (especially if it's an older one) might actually be depending on its "bugs" for proper behavior. Think (and test) hard before applying suggested changes from static analysis.

StavrosK 15y ago
Sure, but the bug in this case is that there wasn't a comment specifying the reason for the unconventional behaviour.
- scott_s 15y ago
  I agree. If you know you're writing something that is un-idiomatic or you think its intended purpose will be a surprise to most readers, put a comment in explaining why. // we need a heap object so we can synchronize on it later Boolean b = new Boolean(true);
  - cdmckay 15y ago
    I'd also make the name a bit more descriptive.
  - groby_b 15y ago
    This points to a deeper problem of static analysis, though - any analysis package without the ability to annotate code is _doomed_. The false positives will be so annoying that people will give up on it. And for the people who work on SA systems - please give me a way to annotate that is not exclusively via comments. Especially once people use multiple SA packages, that is rather annoying :)
- plasma 15y ago
  Don't forget the OpenSSL "bug" (using uninitialized memory) that was really on purpose, resulting in a critical flaw once changed. In that instance too, lack of commenting to explain the behavior was also at fault.
RodgerTheGreat 15y ago
The choice of a Boolean as a monitor object is a little odd. My understanding is that the usual convention is to create such objects via "new Object()", which is a little more obvious- the only reason you would ever call the base Object constructor is to produce something that can be used as a monitor.
- kaffeinecoma 15y ago
  It was a couple of years ago and I no longer have access to the codebase, but the business logic of that piece of code called for a boolean. The problem was they they also (ab)used that field for the lock. It either should have been a separate field (of type Object, as you suggested) or use the existing Boolean, but call it "lock" or "monitor" or somesuch.
dennisgorelik 15y ago
That's a weird line of code to begin with. Not only the naming was wrong, but the type was wrong too (Boolean instead of Object). Could you check all the references where that b variable was used prior to changing the code? There likely to be just one or two such places.
- kaffeinecoma 15y ago
  Sure I could have found all of the references easily. That's one of the reasons I still prefer Java over a dynamic language- my IDE can tell me instantly (option key + F7) where a particular object is used in the entire codebase. I just wasn't careful enough, because hey, what could possibly go wrong- it's a stupid boolean, right?
  - dennisgorelik 15y ago
    When I see unusual error in code - not only I check related code, but I may run svn blame. There are multiple benefits for it: 1) I can learn something new and what I thought was an error would turn out to be an interesting new coding trick. 2) If it was an error - not only we would fix it, but would also learn not to make such error in the future. In particular, that person who made that error would learn to avoid it in the future.
jasonwocky 15y ago
Sure, it wasn't great code, but this also sounds like a flaw in the static analyzer. It should have been able to tell that the variable was being synchronized on, and recognized that using Boolean.TRUE would have been an unsafe change.

StavrosK 15y ago

Sure, but the bug in this case is that there wasn't a comment specifying the reason for the unconventional behaviour.

scott_s 15y ago
I agree. If you know you're writing something that is un-idiomatic or you think its intended purpose will be a surprise to most readers, put a comment in explaining why. // we need a heap object so we can synchronize on it later Boolean b = new Boolean(true);
- cdmckay 15y ago
  I'd also make the name a bit more descriptive.
- groby_b 15y ago
  This points to a deeper problem of static analysis, though - any analysis package without the ability to annotate code is _doomed_. The false positives will be so annoying that people will give up on it. And for the people who work on SA systems - please give me a way to annotate that is not exclusively via comments. Especially once people use multiple SA packages, that is rather annoying :)
plasma 15y ago
Don't forget the OpenSSL "bug" (using uninitialized memory) that was really on purpose, resulting in a critical flaw once changed. In that instance too, lack of commenting to explain the behavior was also at fault.

RodgerTheGreat 15y ago

The choice of a Boolean as a monitor object is a little odd. My understanding is that the usual convention is to create such objects via "new Object()", which is a little more obvious- the only reason you would ever call the base Object constructor is to produce something that can be used as a monitor.

kaffeinecoma 15y ago
It was a couple of years ago and I no longer have access to the codebase, but the business logic of that piece of code called for a boolean. The problem was they they also (ab)used that field for the lock. It either should have been a separate field (of type Object, as you suggested) or use the existing Boolean, but call it "lock" or "monitor" or somesuch.

dennisgorelik 15y ago

That's a weird line of code to begin with. Not only the naming was wrong, but the type was wrong too (Boolean instead of Object). Could you check all the references where that b variable was used prior to changing the code? There likely to be just one or two such places.

kaffeinecoma 15y ago
Sure I could have found all of the references easily. That's one of the reasons I still prefer Java over a dynamic language- my IDE can tell me instantly (option key + F7) where a particular object is used in the entire codebase. I just wasn't careful enough, because hey, what could possibly go wrong- it's a stupid boolean, right?
- dennisgorelik 15y ago
  When I see unusual error in code - not only I check related code, but I may run svn blame. There are multiple benefits for it: 1) I can learn something new and what I thought was an error would turn out to be an interesting new coding trick. 2) If it was an error - not only we would fix it, but would also learn not to make such error in the future. In particular, that person who made that error would learn to avoid it in the future.

jasonwocky 15y ago

Sure, it wasn't great code, but this also sounds like a flaw in the static analyzer. It should have been able to tell that the variable was being synchronized on, and recognized that using Boolean.TRUE would have been an unsafe change.