May 21, 2014 - Building my First Findbugs Detector

Code log entry 084: After roughly 187 minutes of analyzing using the Java Visual VM, the cause of the non-terminating program was found. A lone ExecutorService was found to have never been shutdown(), and the internal thread pool had been keeping the JVM from terminating. A proper call to shutdown() was added to the offending class's stop() method, resolving the issue.

The problem I ran into above was a painful one to track down, but my intuition said the problem would make for a great FindBugs detector because it dealt with omission of code, which is easy for static analysis to detect and generally time-consuming for humans to detect.

What follows is how I designed a detector to hopefully make the struggles detailed in entry 084 obsolete. It is meant for readers who have not yet implemented a FindBugs detector, but are curious at what it entails and what a sample workflow might be.

FindBugs Detector Recipe

Preparation time
15m
Coding time
45m
Difficulty
Derived from existing detector
Serves
the purpose of a nice, easy introduction

When I implemented this detector, I contributed it to fb-contrib. Pedagogically, if you don't have to worry about messing up the actual FindBugs code and you want to focus more on the detector and less on all of FindBugs internals, so I recommend doing the same for your first detector.

This recipe assumes the following conditions:

  • You have the source code for both FindBugs and fb-contrib downloaded and ready to build (I recommend Eclipse)
  • You have read (or at least skimmed) Chris Grindstaff's high level overview of building a FindBugs detector at least once. Don't worry if you don't understand everything, we'll be covering a lot of the topics and, trust me, things get much clearer when you do them yourself.
  • You've read the JavaDoc for Executors (the protagonist of our story) and perhaps browsed this sample code to get a feel for things.
  • You have a JDK installed, and a reference to it in your path.

Setting up our TestBed

If you have ever done Test Driven Development, writing your test code first is not a new idea. For building FindBugs detectors, it is even more important because we need the source code to generate the Java bytecode that FindBugs scans. I haven't found a way to make FindBugs operate in a unit test scenario (i.e. run tests and watch a green bar as they pass), but I still write test cases with code that should and should not work.

Let's make a file, called HES_Sample.java, that will contain some buggy code. This will go in the samples folder when we are done with all the other test cases. I immediately replicated (and simplified) the case that caused me 3 hours of debugging to track down, as well as the fixed version:


        
Lines 5 and 19: I tend to use this //tag and //no tag notation to indicate that FindBugs should put a notification on the next line. Line 30: This call to shutdown() is what we will look for to determine if an ExecutorService has to the potential to ever be shutdown, or not (like the example above) Line 34: I made a dummy class to save on typing lots of anonymous classes.

For those of you thinking ahead, there are (at least) two more cases where ExecutorServices can be left hanging, or be misused, which I'll cover in a later blog post.

Finding some detector code to work with

Chris Grindstaff recommends finding a similar detector to base our new detector off, and I heartily agree, especially for the first, second or nth detector that you plan to write.

There is no magic way to find a similar enough detector, but I've had luck browsing through the fb-contrib messages.xml file until I find a message that seems familiar. I, by a stroke of luck, had been recently improving the message on the PMB_PossibleMemoryBloat detector, which seemed like a good start. It seems suitable because it detects collections that never get a call to remove(), clear() or similar methods (and thus always get larger but never get smaller), which is similar to what we want to do.


        
fb-contrib is released under the LGPL Most detectors extend ByteCodeScanningDetector. It gives us a framework for analyzing bytecodes without having to worry about parsing things ourselves. This is a list of Collections that PMB checks. Why the L/ out front? Think of that as they fully qualified compiled name of each class. This is a list of methods that remove things from collections. This is a list of methods that add things to collections. XFields are objects representing fields. FieldAnnotations point to the line number where the field is created, so we can put our notification in the right place. This is one of the visitor methods mentioned below. This is one of the visitor methods mentioned below. This is one of the visitor methods mentioned below. This is one of the visitor methods mentioned below. This is a helper method. We'll cover it later. This is a helper method. We'll cover it later. This is a helper method. We'll cover it later. This is a helper method. We'll cover it later. This is a helper method. We'll cover it later.

You clever people in the audience may have guessed that to make our HangingExecutors detector work, all we will need to do is copy most of this code into our detector and modify bloatableSigs from line 8 and decreasingMethods from line 15 to our ExecutorService specific signatures and we'd be basically done. You clever folk would be correct.

Visitor Pattern Interlude

Before we get into how to modify this code to suit our needs, let's go over the interesting parts, that is, the snipped out code and see exactly how this detection thing works. Before we get to it, I'd like to give a quick refresher on the Visitor pattern because the pattern is how detectors function -- FindBugs uses BCEL (JavaDocs) to walk through the compiled Java bytecode and creates "events" every time it runs into things, the beginning of a class or an individual bytecode, for instance. Our job is to override these listeners and keep track of the interesting bits to detect our bugs.

Generally, you will need the following four visitor methods to make a detector (There are many more than these four, but these will do most of the job). The order that these get called is:

That is, for every class, visitClassContext() is called [in this method we tend to look at fields), then for every method, we get a visitMethod() [usually to get the name or reset the OpcodeStack], then a visitCode() [similar to visitMethod()], and then for every opcode, a call to sawOpcode() is performed [the meat of most detectors goes here].

How does PossibleMemoryBloat work?

At a high level, PMB works in four steps:

  1. Find all Collection fields and put them in bloatableCandidates.
  2. While parsing through all methods, see if things ever get added to the Collection fields and add those to bloatableFields.
  3. While parsing through all methods, remove any fields that get a call to clear(), remove() or similar.
  4. Report bugs on all remaining Collection fields.

This seems straight forward enough. Because ExecutorServices are just dangerous if they don't get shutdown, our detector will omit step 2, but I'll still walk through the original PMB source code for that step.

The following source code handles step 1, the discovery of Collection-based fields:


        
Parses the fields defined in this class This call to super.visitClassContext() is very important. Without this call, the traversal of the class stops, meaning none of the other visitor methods will be called. It's pretty typical to force any fields back to null for garbage collection to occur after you are done with the class context. This is how you can access all the fields in a class. Note that Field is the BCEL object (read: not FindBugs message compatible) and XField, created on line 39, is the FindBugs version (which adds some extra functionality). The detector only looks at static fields, which will be around for longer than local variables, and thus more prone to bloat. When at all possible, it is best to convert the BCEL objects (like Field, Method, ClassDescriptor) and turn them into their FindBug's equivalent (XField, XMethod and XClass) using the convenience methods in XFactory Detecting local instances of ThreadLocals is also wrapped into the PMB Detector. When we reuse this detector, we'll be able to omit this, but for now, it's a good example that one detector can be responsible for more than one bug type. To report a bug, simply call reportBug() on the bugReporter with a new BugDetector. For now, don't worry about the this, just know that we need to have it and the name of the bug. NORMAL_PRIORITY actually refers to confidence (!), which impacts severity. We can customize the displayed message to include things like the class name. The order matters, so this class annotation is referred to as Annotation Index 0 To make the message include the name of the field and make bug appear at the right line number, we need a field annotation, which we cached in parseFields(). The order matters, so this field annotation is referred to as Annotation Index 1. Other techniques for specifying line number include using SourceLineAnnotation.

The rest of the visitors are shown in the next code snippet. sawOpcode() does most of the work. A word of warning, some of the sawOpcode() methods in other detectors contain really long switch case methods and can be a bit daunting at first, but be strong! Carry on!


        
Remember this Method object is from the BCEL framework. This method is more of a 'notification' call than the start of a deeper visitor pattern traversal, so, we don't need to call super.visitMethod() to continue traversing (in fact, the overridden implementation is blank) At first blush, this method may seem similar to visitMethod(), but the key difference is that visitCode() is called as part of actually parsing the bytecode (so we need to call the super's implementation to continue parsing) This is a common way to avoid doing anything in the constructor or the static initializer. We'll be using the OpcodeStack soon, so we need to reset it to clear it out. Each of these opcodes will be something off this list. Thankfully, you won't have to have a bunch of magic numbers floating around your code because these are all defined in a superclass. This call is probably redundant given lines 13 and 14, but it's probably better to be safe than sorry. This is needed for the OpcodeStack to collect information before something is pushed or popped. INVOKEVIRTUAL and INVOKEINTERFACE are two of the five bytecodes that can be used to indicate a method. Apparently we don't need to worry about the other three. The built-in getSigConstantOperand() returns a semi-cryptic String that indicates how many params a method call had. For example, "(JLjava/util/concurrent/ TimeUnit;)Z" means that a long and something of type type java.util.concurrent. TimeUnit were passed in. Type.getArgumentTypes() parses this into usable Objects. At this point, the stack we have looks like this and we want to get to the name of the method. So, we have to dig down past the number of arguments and get that stack item. Rather than casting Items we pull off the stack, the convention is to call getSomething() and checking the result against null. If a field is returned, we (using static analysis) can't be sure that elsewhere, something like clear() is **not** called, so, we remove a potential false positive because following the returned value is challenging. In all cases, we want to add the opcode to the stack (so the stack works like we expect). Why do it in a finally block? It's a nice safe way to make sure it will always happen, even if we happen to continue in one of the above branches. Questionable practice? Maybe. Effective? Yes. Evidence that the author of the detector copied and pasted from elsewhere? Almost certainly. If this is an increasing method (something is added to the collection), we move the field from the candidate list to the culprit list. Otherwise, if the method is a decreasing one, remove the field from both lists. If the current opcode deals with returning something, the thing to be returned is at the top of the opcodeStack. No need to count arguments or anything like that. getNameConstantOperand() a built in method that gets us a relevant name that is associated with the current opcode. Because the current opcode deals with a method call, we know this will be the method name.


So that was all the code in the detector. Not trivial, but not so bad once you know a couple of FindBugs quirks, eh? Some of you may be wondering about the message that gets displayed. That involves three more files and is more of a bureaucratic step than a development step, therefore I'll kill two birds with one stone and explain the "paperwork" concepts while we make our own detector in the next section.

Building our custom "no shutdown() calls on an ExecutorService" detector

Making the message and other bookkeeping

Before we make our code, we'll need to make a message and tell FindBugs how to invoke our detector. That will involve adding entries to three files, etc/bugrank.txt, etc/findbugs.xml, and etc/messages.xml.


        
This is our added line, appended to the bottom (order doesn't matter). Be very careful not to omit a space. This will make FindBugs crash because of this line of code, which can lead to a terrifying wild-goose chase.

That new line of code [line 9 in the example] registers a new BugPattern called HE_EXECUTOR_NEVER_SHUTDOWN. A BugPattern is tied to a message and lives in exactly one Detector. A single detector can be responsible for one or more BugPatterns. We've already seen an example where one Detector was responsible for two BugPatterns.

What's the -2 for and why do all the others have 0? Well, that number modifies the severity of the bug that is reported. The severity follows the following formula:

Strictly speaking, adding the BugPattern to bugrank.txt is optional, because if there isn't an entry, the second modifier defaults to 0. PMB, as are a plurality of the bugs, is a "Correctness" bug. Our custom detector will also be in this category.

Now, to tell FindBugs that we have made a new detector and where to find it, we go to etc/findbugs.xml, and add two entries:


        
We need to add this entry to the XML. Note that we have our class name and one or more detectors that the class can report. We haven't made com.mebigfatguy.fbcontrib. detect.HangingExecutors yet, but we will shortly. And now we need to register the existence of the BugPattern we are making. Don't worry too much about the abbreviation part, just make something with three letters (to avoid conflicts with the two-letter FindBugs abbreviations. It's good practice to set new detectors to be experimental.

The final part, and arguably the most important part from a usability standpoint, is making an understandable message. This, of course, goes in etc/messages.xml, along with a description of the detector itself and one last BugCode. To explain the difference between the BugPattern message and the Detector description, see the following image:



        
This is where we add in our detector description (and need to repeat our class name). The meat of your description goes in the CDATA comment. You can use html if you wish. Here is the all important BugPattern message. Note the three parts. They are all important. It is common practice to have placeholders in the LongDescription. Remember earlier when we made a call to addField()? That will replace the {1} when the string is rendered (see the above image for an example. Your error message goes in the CDATA comment. Feel free to be verbose, or put code samples in <code> tags, whatever you want. As of FindBugs 2.0.3, you cannot, unfortunately, have injected content like in the LongDescription. Finally, add an abbreviation for your detector. This will be used in the descriptions of the bug.

And that wraps up the paperwork. Now, let's go finish writing the detector!

The modified code

Since we've already seen the meat of the detector, just in a slightly different form, I won't go into as granular of detail as I did earlier. The resulting code is below, with a few remarks on the changes.


        
Again, this is just the fancy "full" name for the ExecutorService, which are produced by the Executors class. These are the only two methods that can safely shutdown the ExecutorServices and the Threads inside. I've elected to simplify the detection process a bit, by removing second Map that we previously transferred the candidates to. This has also removed the set of increasing methods. All we need to do is remove any fields that have a call to shutdown, and once we get to the end of the class, all that remain are the non-shutdown ExecutorServices.

Finishing up

Now, to compile the plugin, just run the build script that's a part of the fb-contrib source. It's an ant script, so it will take care of everything (compilation, dependencies, etc). The only thing you may have to do is to download yank.jar from Maven to act as the dependency manager and put it in your .ant/lib folder, which for me was in [HomeDirectory]/.ant/lib/ .

Finally, put your newly built fb-contrib.jar into the findbugs/plugin dir (for me, it was [eclipse install dir]/plugins/edu.umd.cs.findbugs.plugin.eclipse_2.0.3.20131122-15027/plugin/). Run Findbugs and marvel in your cleverness!

Epilogue

Congratulations! You've written your first FindBugs Detector. From here, the door only opens wider. Future posts will cover more advanced topics, like OpcodeStack gymnastics and looking at the bytecodes for inspiration to write a detector. Check back soon!