So You Want to Practice your Code Reviewing Skills?

The code below is taken from a real web-app that has been up and running for > 7 years. This specific fragment is realizing the visitor count functionality: keeping track on the number of visitors hitting the site. Each time a new session is created SiteInfo.instance().addSession() is called.

Your task (should you choose to accept it...) is to find bugs in this code. In other words: will the visitor count, (as reported by SiteInfo.visitorCount()) always be correct? If not, what values can be seen there? How can we fix the code?

Please ignore design issues (I don't like the singleton anymore than you do), or technological issues, such as: "well you should just rewrite the whole thing with Spring + Hibernate". Just examine the code-as is and try to determine if/where can it fail.

public class SiteInfo {
   
   private static SiteInfo inst;
      
   private SiteInfo() {
      //
   };
   
   // This is a singleton (Yak!)
   public static SiteInfo instance() {
      if(inst == null) 
         inst = new SiteInfo();
      return inst;
   }

   private int sessionCounter = 0; 
   private static final String INIT = "100";
      
   public int visitorCount() {
      return sessionCounter;
   }
      
   public synchronized void reloadParamsFromDB() {
      Connection con = null;
      ConnectionPool pool = null;
      try {

         pool = ConnectionPoolFactory.getInstance();
         con = pool.getConnection();

         String countStr = DataBaseHelper.get(con, "config", "value",
            "param", "session_count"); 
         if (countStr == null) {
            String q = "INSERT INTO config VALUES ('session_count','" + INIT + "')";
            try {
               runCommand(q, con);
               countStr = INIT;
            }
            catch (Exception e) {
               log.error("", e);
            }
         }

         sessionCounter = Integer.parseInt(countStr);
      }
      catch (Exception e) {
         log.error("", e);
      }
      finally {
         ConnectionPoolFactory.release(pool, con);
      }
   }
   
   private void runCommand(String q, Connection con) throws Exception {
      Statement stmt = con.createStatement();
      try {
         stmt.executeUpdate(q);         
      }
      finally {
         stmt.close();
     }
   }
   
   public synchronized void addSession() {
      sessionCounter++;
      if(sessionCounter % 100 != 0)
         return;

      Connection con = ConnectionPoolFactory.getInstance().getConnection();
      Statement stmt = null;
      try {
         stmt = con.createStatement();
         String sql = "UPDATE config SET value='" + sessionCounter 
            + "' WHERE param='SESSION_COUNT'";
         stmt.execute(sql);
      }
      catch (Exception ex) {
         log.error("", ex);
      }
      finally {
         try {
            if(stmt != null)
               stmt.close();
            ConnectionPoolFactory.release(con);
         }
         catch(SQLException e) {
            log.error("", e);
         }
      }
   }   
}

public class DataBaseHelper {
   public static String get(Connection con, String table, 
      String columnA, String columnB, String columnAValue) {
      String result = "";
      String sqlQuery = "SELECT " + columnB + " from " + table
            + " WHERE " + columnA + " = '" + columnAValue + "'";

      // Run the query. Translate the result set into a list of maps.
      // Each map corresponds to a single row in the ResultSet
      List<Map<Object,Object>> rows = generateResult(sqlQuery, con);
      try {
         Iterator<Map<Object,Object>> iter = rows.iterator();
         if(iter.hasNext()) {
           Map<Object,Object> m = iter.next();
         
           result = (String) (m.get(columnB));
           if (result == null)
              return null;
         }
      } 
      catch (Exception e) {
         return null;
      }
      return result;
   }  

   public static List<Map<Object,Object>> generateResult(String query, Connection con) {
      List<Map<Object,Object>> result = new ArrayList<Map<Object,Object>>();
      try {
         ResultSet resultSet = DataBase.performSqlQuery(query, con);
         if(resultSet == null)
            throw new Exception("Impossible"); 
            
         ResultSetMetaData resultSetMetaData = resultSet.getMetaData();         
         int columnCount = resultSetMetaData.getColumnCount();
         
         String[] columnNames = new String[columnCount];
         for(int i = 0; i < columnCount; i++)
            columnNames[i] = resultSetMetaData.getColumnName(i + 1);
            
         while(resultSet.next()) {
            Map<Object,Object> map = new HashMap<Object,Object>();
            for(int i = 0; i < columnCount; i++) {
               String col = columnNames[i];
               map.put(col, resultSet.getString(i + 1));
            }

            result.add(map);
         }
      }
      catch(Exception e) {
         sLogger.error("", e);
      }

      return result;
   }
}

JUnit Rules!

Rules are a simple, yet amazingly powerful, mechanism introduced in JUnit version 4.7. They allow developers to easily customize JUnit's behavior by exposing meta information regarding the currently executing test. This post provides a straightforward example for writing a custom rule that augments JUnit with some useful functionality.

My subject class is IntSet: A set of integers implementing the standard operations of add(), remove(), contains(), clear() in O(1) time. To make this performance guarantee the set needs to know (in advance) the range of the values (min..max) and its size limit (number of elements that it will accommodate).

All in all, IntSet looks something like this:

public class IntSet {
  ... // Some private fields
  public IntSet(int limit, int min, int max) { ... }
  public int size() { ... }
  public boolean contains(int n) { ... }
  public void add(int n) { ... }
  public void remove(int n) { ... }
}

One of my unit tests specifies the behavior of IntSet when its size limit is reached. If I'm only interested in the type of the exception I can specify it via the expected attribute of the @Test annotation:

  @Test(expected=IllegalStateException.class)
  public void shouldNotExceedCapacity() {
    IntSet s = new IntSet(2, -10, 100);  // Set size limit to 2
    s.add(30);
    s.add(40);
    s.add(50);   // Insertion of the 3rd element should fail
  }

There are two drawbacks with this test. First, It only asserts the type of the an exception. It does not check the error message specified for the exception. Second, it does not assert that the exception was triggered by the last add() call. In other words, if we have a bug and the 2nd add() call is failing - with the same type of exception - the test will still pass.

To overcome this limitation we want to check the error message of the thrown exception. Specifically, we want to verify that the execution of the method fires an exception whose error message is "Cannot insert '50' - The set is full". Clearly, the chances of such an exception being thrown by the 2nd call are pretty slim.

Extending JUnit in such a manner is pretty easy thanks to the rules mechanism:

public class IntSet_Tests {

  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.METHOD)
  @interface Throwing {
    public String value();
  }
  
  @Rule
  public MethodRule mr = new MethodRule()
  {
    @Override
    public Statement apply(final Statement base, FrameworkMethod m, Object o) {
      Throwing t = m.getAnnotation(Throwing.class);
      if(t == null)
        return base;
      
      final String message = t.value();
      return new Statement() {
        
        @Override
        public void evaluate() throws Throwable {
          try {
            base.evaluate();
            fail("No exception was thrown");
          }
          catch(AssertionError e) {
            throw e;
          }
          catch(Exception e) {
            assertEquals("Incorrect exception message", message, e.getMessage());
          }
        }
      };
    }    
  };

  // All sort of @Test methods ...


  // And now, a method that asserts the error message
  @Throwing("Cannot insert '50' - The set is full")
  @Test
  public void shouldNotExceedCapacity() {
    IntSet s = new IntSet(2, -10, 100);
    s.add(30);
    s.add(40);
    s.add(50);
  }
}

First we define a new annotation, @Throwing. Then we define a field annotated with @Rule to provide the custom handling of this annotation. Finally, we annotate the shouldNotExceedCapacity() method with a @Throwing("Cannot insert '50' - The set is full") annotation.

The mechanism works as follows: before each test method is run, JUnit creates a Statement object which is merely a command object through which the acutal method can be invoked. JUnit passes this object along with a FrameworkMethod object (a wrapper of Java's Method) and the unit test instance to all @Rule fields defined at the test class.

A @Rule field must be public and must implement the MethodRule interface (of course, you can instead extend one of several classes conveniently defined by JUnit). In the apply() method, above, we create a new Statement object that wraps the original one. The new evaluate() method will check that if an exception is thrown its message matches the text specified by the @Throwing annotation attached to the method.

Obviously, there are other ways to do that. For instance, one can use the ExpectedException class (a predefined JUnit rule) to achieve a similar effect. The purpose of this post is to surface the (mighty) powers of JUnit meta programming.

Axiom: Instability

I had already blogged about the Axioms of programming: the fundamental rules that govern the development of every piece of (substantial) software. In this post I want to focus to on the Instability Axiom:

The external behavior of a component will need to change over time

Here's a real story. I once was involved with a very a small project, let's call it the PQR project: three people working part time for three weeks, putting together an HTTP server a web client and a GUI client - all are very simple. During these three weeks we were also learning some new technologies so actual coding time (of all developers combined) was about 20-25 days.

During these three weeks two important changes were applied to PQR's specification:

• The technology with which the GUI client was implemented had to be changed. Instead of implementing it over Tech.A we had to switch to Tech.B.
  
  
• The initial specs defined the data that should be persisted by the server. As we were playing with the intermediate versions of the project, we came to realize that a descent user experience requires that additional information will be persisted.
  

The main point of this post is not if/how we managed to support these changes. The point is that even in small projects specs are not stable. We were not successful in defining the project's goals for a three week period in a project which is as simple as industrial projects get. Of course, If the project were more complicated (more people, wider scope) then the instability was likely to be even higher.

This example indicates that a "fire and forget" type of development (AKA: "divide and conquer" ) where one breaks down the desired functionality into a few large pieces, assigns each piece to a programmer, and then lets each programmer work on his task in isolation of his peers (until an "integration" milestone is approaching) is broken.

First, external forces will change the specs, thus affecting the assigned tasks. In the PQR project, the change in client technology was due to some external factors (business/marketing constraints). Even though the initial specs were examined and audited by several layers of approvals, no one had predicted this change.

Second, feedback from working early versions of the product (even with partial functionality) will change our understanding of the product and its desired capabilities. In PQR, the change regarding which-information-should-be-persisted was driven by experimentation with early versions.

If we had taken a Fire-and-Forget approach then our ability to respond to the first change were very limited as every team member was in the middle of his large task. Also, by the time a first working version were available, very little time was left to implement significant changes.

Bottom line: Software is unstable. Breaking the effort into tiny tasks with frequent integration points (I am speaking about granularity of hours not weeks) is an excellent way to cope with this inherent instability.

Programmers are Decision Makers

So yesterday I bumped into this bug: a listener of a tree object (not Swing's JTree. A tree with nodes and labeled edges, a-la Data Structures 101) was referencing a null object:

public class Auditor implements TreeListener {

  private Map<String,Date> map = new HashMap<String,Date>();

  public void nodeAdded(Node n) {
    map.put(n.id(), new Date());
  }

  public void nodeRemoved(Node n) {
    Date d = map.get(n.id());
    String s = d.toString();  // <-- NPE is here!!
    map.removeKey(n.id());

    // some other things ...
  }
}

My unit tests told me that the Auditor and Tree class are working fine - the tree fires the correct events and the Auditor reacts correctly. Of course, a Green bar does not guarantee "no bugs" so I double checked the code and it seemed fine.

Indeed the problem was elsewhere. Turns out that a bug in my app caused each listener to be registered twice. Auditor was not expecting to be called twice for each event, hence the NPE.

So I fixed the bug. Then I went back to the addListener() method of the Tree class. It is a very simple method:

public class Tree {

  private List<TreeListener> listeners
    = new ArrayList<TreeListener>();

  public void addListener(TreeListener arg) {
    listeners.add(arg);
  }

  // Additional methods ...
}

Looking at the code I tried to think if this method should be changed in order to prevent future occurrences of this bug. As I was thinking about it I realized that this simple method is an excellent illustration to one software's most fundamental truths:

Programming is about making (design) decisions.

I am not the first one to say this (see Joakim Holm's "Programming is all Design" or Alistair Cockburn). Here's the (probably partial) list of decision that a programmer has to make when writing a one-liner method that should simply add a listener to a list.

1. Should the listeners field be private? Maybe we want subclasses to manipulate/inspect it in some way, which calls for protected visibility.
   
   
2. What is the order of notification? Should we use a "first-registered first-notified" policy? or maybe "last-registered first-notified". Is order important at all? If not then maybe we would like to force the application not to depend on the order by random shuffling?
   
   
3. Can a listener be registered with more than one Tree object? If so, then the listener interface should probably specify the originating tree, not just the node
   
   
4. Can a listener be registered more than once with a single Tree object? Usually the answer is no, but there are scenarios where multiple registration does make sense.
   
   
5. Assuming each listener may be registered exactly once, how do we deal with multiple registrations?
   
   • Throw an exception?
   • A standard Java assertion? Could be disabled which may be good or bad depending on context.
   • Throw an AssertionError? Has a more dramatic effect than a plain Exception. Also will be reported by JUnit as a failure rather than an error. Again, may be either good or bad.
   • Silently ignore. Return from the method without adding the listener. This seems very natural but it has the drawback that it hides the problem. If I choose to silently ignore then I will get no notification about future problems. How about logging?
   • Return a Boolean value indicating whether the listener was already registered. Puts the responsibility on the client code to check this value. Will not always happen.
     
     
6. Do I need to provide some thread protection?
   
   • Define the method as synchronized?
   • Define the listeners field as a ConcurrentLinkedList?
   
   
7. How should this list of listeners interact with the Garbage Collector? Should a listener that is only accessible from this list be collectible (by storing it as a WeakReference)? A "No" answer puts additional burden on client code: must remove listeners from the Tree object when they are no longer needed. If listeners are dynamically added as the app is running this may become a big problem.
   
   Note that choosing weak references is not always a good idea. Some listeners are only accessible from the listened-to object. Take for example a listener that inspects the tree and updates the title of the main window. It is created, pushed onto the listeners list and all other traces to it are lost. A weak reference will make such a listener disappear with the very first collection cycle.
   
   
8. Do I want to refactor the whole "listeners" concern into a dedicated strategy class. This will improve testability and decoupling.

As I said this is a partial list. Point is simple: a programming task requires much more decision (and mental energy) that what initially seems. In other words: If you want to design everything upfront you'll end up programming upfront but without the assistance of tools such as IDE, Unit tests, Refactoring, and the likes. Good luck.

Katacast: String Calculator – Groovy

The original idea behind Code Katas was that of a small programming exercise whose solution often includes some interesting challenge. In katacasts.com Corey Haines further explains that:

Over time, the concept of Katas grew from a problem to solve to a solution to practice. Uncle Bob Martin (among others) began talking about the idea of practicing the solution to a kata until the steps and keystrokes became like second nature, and you could do them without thinking.

In TDD Katas the focus in on doing the TDD cycle, Red-Green-Refactor, right. Corey had recently published a screencast where yours truly solves the StringCalculator TDD Kata in Groovy. It illustrates some points about TDDing (like "Fake it till you make it" or "triangulation") in a very clear way.

So, If you feel like you want to better understand TDD you can watch the Kata on Corey's site. An explanation of the different moves is given in the accompanying text.

Another recommendation is to watch Gary Bernhardt's screencast where he refactors cyclomatic complexity code in Python.

Enjoy.

Swing - Tabs to Spaces

How do you make a swing text component (JTextEditor, JEditorPane, etc.) to insert spaces whenever the user hits the "Tab" key?

The answer is below. It is based on setting a document filter on the component's document object. The (static) install() method can do all the setup for you.

package com.blogspot.javadots;

import javax.swing.text.AbstractDocument;
import javax.swing.text.AttributeSet;
import javax.swing.text.BadLocationException;
import javax.swing.text.DocumentFilter;
import javax.swing.text.JTextComponent;

public class TabToSpaceFilter extends DocumentFilter {
   
   private String spaces = "";
   private JTextComponent textComponent;
   
   public TabToSpaceFilter(int tabSize, JTextComponent tc) {
      textComponent = tc;
      for(int i = 0; i < tabSize; ++i)
         spaces += " ";         
   }
   
   @Override
   public void insertString(FilterBypass fb, int offset, String text,
      AttributeSet attr) throws BadLocationException {
      super.insertString(fb, offset, translate(offset, text), attr);
   }

   @Override
   public void replace(FilterBypass fb, int offset, int length,
      String text, AttributeSet attr) throws BadLocationException {
      super.replace(fb, offset, length, translate(offset, text), attr);
   }

   private String translate(int offset, String s) {
      int col = columnOf(offset);
      
      StringBuilder sb = new StringBuilder();
      int top = s.length();
      for(int i = 0; i < top; ++i, ++col) {
         char c = s.charAt(i);
         if(c == '\t') 
            sb.append(spaces.substring(col % spaces.length()));
         else
            sb.append(c);
      }   
      
      return sb.toString();
   }

   private int columnOf(int i) {
      String s = textComponent.getText();
      if(i == 0)
         return 0;
      int prev = s.lastIndexOf("\n", i-1);
      if(prev < 0)
         return i;
      
      return (i-prev)-1;
   }
   
   public static void install(int tabSize, JTextComponent area) {
      AbstractDocument ad = (AbstractDocument) area.getDocument();
      ad.setDocumentFilter(new TabToSpaceFilter(tabSize, area));
   }
}

How JUnit's assertArrayEquals() should be implemented

I find myself writing this assertion method at (almost) any project where #tests > K where K is typically 10. In other words: Every project.

   public static void arrays(Object[] actual, Object... expected) {
      for(int i = 0; i < Math.min(actual.length, expected.length); ++i)
         Assert.assertEquals("Array mismatch at index " + i + ":", expected[i],
            actual[i]);
        
      Assert.assertEquals("Array length mismatch", expected.length,
         actual.length);
   }

Top Five Bugs of My Career - Solved

Yes, I know. A long time has passed since the "top five bugs" post, leaving these bugs waiting to be resolved (to my defense let me say this is not intentional between my day job and me writing my Ph.D. dissertation I have very little energy left for blogging).

Anyway, the long wait is over. It is now solution time.

#5: Only guests are welcome - 2008, Javascript

The night before I did some refactoring. I had two methods, let's call them f1() and f2(), both taking two parameters and doing almost the same thing. So I consolidated them into one method, f(). I then searched all call sites of f1() and f2() and changed them to call f(). One of the f2() calls passed null as the second parameter, however f() was built mainly on the f1() code so it didn't know how to handle nulls.

This was frustrating because there were very few f2() calls and I went over them manually. Actually, at some point the null was just there in front of my eyes. But at that moment I didn't think that f() cannot digest nulls. I didn't bother to write unit tests for the Javascript part of the project, so I had no way to verify my refactoring. #Lazy

#4: The unimaginably slow load - 2005, Java

When I wrote the code that saves the data structure into a file (last step of the import) I chose the simplest thing that could possibly work: serialization. Later, I added a publish/subscribe functionality to the data structure, thereby allowing GUI objects to respond to changes in the data.

Guess what happens when you serialize a data structure that has a reference to a listener that is an inner class of the app's Top-level JFrame object: you serialize every piece of runtime data your program holds. Everything. Instead of the saving a minimalistic storage-oriented data structure you suddenly save caches, in-memory indexing tables, the works.

Unfortunately, all these objects were all Serializable so I didn't get any exceptions in the process, just a very slow save/load process.

#3: Modal smiles, Modeless cries - 2001, C

This is a C-specific issue. The error messages were stored in an array of structs where each struct represents a single error. There was an upper limit on the number of errors so I declared an array of 1,000 structs (which was well beyond the limit) . In order to avoid the hassle of dynamic memory management this array was actually a local, stack stored, variable.

After filling the array the code pops-up the Modal window that shows the errors by issuing a DialogBox() call. This call will return only after the window is closed. Thus, as long as the window was open execution was still sitting there at my function, and my array was safe on stack.

A Modeless window is different. The call that creates a modeless window, CreateDialog(), returns immediately while the window is still open. Execution then continues, reaches the end of my function and returns to the caller. At this point, the stack is unwound and the array vanishes into thin air. When the user clicks on an error the event handler tries to access the array (via a pointer) but the array is no longer there. It is an ex-array.

#2: The out-of-nowhere crash - 2002, C++

Again, we are in the no-garbage collection territory. In C++ you constantly write destructors that take care of releasing the resources that the object acquired. This is a safe way to avoid leaks.

What happens when you have a bug in a destructor (e.g.: null pointer error)? Right. the program crashes.

When are destructors of local variables called? At the end of the scope. Literally. At the curly brace character closing the scope.

So there you have it: the program crashes when it reaches the curly braces after the "b = true". The assignment itself went fine. The destructor was the problem. My IDE back then was not smart enough to treat closing braces as a statement. It treated it as part of the "b = true" assignment thereby misleading me to think the assignment failed.

#1: The memory monster - 2004, C#

Actually, this bug is very simple once you see the code. It is my #1 due to the long time it took to track it down. Let's take another look at the code:

for(int i = 0; i < rows.Length; ++i)
if(rows[i].isOutOfView)
 rows.Remove(i);

If the isOutOfView property is true we remove the current element from the collection. Let's play with i=4. The if() is true, we call rows.Remove(4), we increase i's value to 5 and start the next pass. Alas, the rows.Remove(4) call moved every element one position towards the beginning: the element at position 5 is now in position 4. At the next pass when i=5 we examine the element that used to be at position 6.

Net result: we never checked the element at position 5. Thus, some of the elements that were needed to be removed from the collection were not removed. Later, the isOutOfView property of these elements were reset back to false. Overtime they accumulated until the heap collapsed.

That's it. Hope you enjoyed this account. Other bug stories will be happily acknowledged.

Write only Fields ?!

Java's MutableTreeNode offers a setUserObject(Object) method. Its natural counterpart, the getUserObject() method is defined only by the implementing subclass DefaultMutableTreeNode.

This means that whenever your logic needs to work with the data that is associated with a tree node, it needs to downcast not to the interface, but - wait for it - to the actual implementation. This counters a fundamental engineering principle that Java is often trying to promote: interfaces over implementation.

Moreover, offering a setter without a getter suggests that someone in Sun thought that it might be useful to have a variable that can be written-to but not read-from. This makes as much sense as placing 'something' in a safety deposit box and then throwing away all keys. Why would you want to do that? You might just as well throw away that 'something'. Either way, you'll never see it again.

Done Ranting.

Top Five Bugs of My Career

This time, I thought to take a different angle. Instead of reasoning about software I'll just write about specific acts of programming in specific programs. More precisely, I want to write about specific bugs that I had the (mis) pleasure to track down.

I took my first full-time programming job ten years ago, almost to the day. During this period I introduced and solved many bugs (Hopefully the difference between #introduced and #solved is not very large). My criteria for choosing which bugs will make it into this list is that of memorability - I chose the bugs that I remember most clearly. When discussion bugs, memorability is a key factor. The more painful the bug the greater the impact it will leave in your memory. Thus, in some sense, I am about to list my most painful bugs from the last decade.

To make things interesting, this post will disclose (mostly) the descriptions of the bugs. The cause/solution will be disclosed in the next post. This will give you programmer's mind something to chew on for a few days. For the same reason I also (occasionally) omitted pieces of information whenever I felt that their inclusion in the bug description will make the solution too obvious.

#5: Only guests are welcome - 2008, Javascript

So my partner and myself are about to demo a web app which we quickly prototyped in a couple of weeks. It is an on-line discussion system. Buzzwords used: database, web-server, Ajax, CSS. We also had a decent suite of tests.

It is 8:45am. The demo will start in 15 minutes. I am doing one last quick run of the demo. I log in as an anonymous guest. I browse through the pages and everything is fine. I try to log in as a user. Name: "Noga" (that's my dog's name). Password: "hithere". Oops. No response. Not even a "login failed" message.

I know that login is doing some Ajax-ing so I quickly try to bypass the Ajax mechanism by manually forming a login URL containing my credentials. It works. So I now know that the defect is Ajax related but I don't have the time to rewrite the login page such that it will not use Ajax, let alone test it.

8:55. The Anxiety-meter is screaming. We come up with a cunning solution. We start the demo with an already logged-in user (by secretly entering the login URL before the demo starts). We walk through all the pages. Then we log out and show that everything works for a guest user. Luckily, we are not asked to go back to a non-guest user. We are saved.


#4: The unimaginably slow load - 2005, Java

I am working on a Java mass analysis tool. It is a program that digests jar files and classifies the classes therein based on a set of rules. The program is a console app which supports operations such as these:

• import: import a new jar file into the system and give it the specified name
  
• load: load a previously imported jar-file into the memory
• classify: Run the classification rules on all currently loaded jars. Saves the results into the specified file (.csv)

The import command scans the jar file and transforms it into my own data structure which is optimized for the type of analysis I need to do. The data structure is then saved to the file system. The load command reads the saved file back from into memory.

For a few days I am working on adding new classification rules. When developing new rules I tend to work with a set of small Jars. This minimizes the classification time, thus shortening the feedback cycle. From time to time I find myself also changing the implementation of the underlying data structure (fixing bugs, adding new operations which are needed by the rules, etc.).

At some point I decide that my new rules are complete and I put them to work on several real inputs: large zip files with > 100K classes. I notice that both the import and load commands run much slower than before. In fact, they run so slow that they wipe out all the benefits of having my own optimized data structures.

#3: Modal smiles, Modeless cries - 2001, C

A Win32 Content editing app. User can add, edit, search for, browse through, and delete records. There is also a Record Checker mechanism that scans all records and detects all sort of irregularities such as broken cross-record links, duplicate names, etc.

The output of the checker is a list of error messages. The messages are displayed on a new window. Clicking on a message opens the corresponding record in the main window.

Originally the error window was a modal window: it disabled the main window. Clicking on an error closed the output window and reactivated the main window. We then decided that it makes much more sense to make this a modeless one. I implemented this change and was astonished to see that the click functionality stopped working. Clicking on an error (in the modeless window) created an access violation and a total eclipse of the app. Remember, we are speaking about the C programming language, so expecting something as fancy as a stacktrace is out of the question.


#2: The out-of-nowhere crash - 2002, C++

A C++ Win32 app. I am initiating a shotgun surgery that takes a few days to complete. Back then I was unaware of refactoring/unit testing (the whole company relied on manual testing) so instead of taking baby steps, I did a massive cross-code revolution.

Having battled numerous compilation and linking errors I am finally in a position where I can run the code. The thing immediately crashes. It does not even show the window. Again, the tools that we used didn't support stacktracing. I had no idea where to start.


For those unfamiliar with the Win32 technology, here's some background. In a Win32 app all GUI events of a window arrive at a single callback function which takes four parameters: hwnd - a handle to the originating widget, msg - an int specifying event type, wparam & lparam - two ints providing event specific data.

Typically, the body of such callback functions was a long switch (on the msg parameter) with a case for each of the events that the program was interested in.

In this particular program the message-handling switch block was particularly long. The GUI was quite complicated and there were numerous events (~ 200) that had to be listened to. The callback function was more than a thousand lines long.

First, I tried to apply reasoning. I made educated guesses regarding which events are likely to be the ones causing the crash. After several hours of unsuccessful guesses I went to a more brutal approach: I commented out the whole switch block. This made the crash disappear but eradicated every functionality that the program had. Then I uncommented half of the cases inside this switch block. The crash didn't appear and some functionality went back on. This meant the the crash was due to the code that was currently commented.

I continued the comment/uncomment game using a binary-search strategy. Quite quickly I zeroed in on the problematic message. I placed a breakpoint and started stepping through/into the instruction. This particular switch invoked code on other functions. One of them looked like this:

bool b = false; 
if(...) {
  // many lines
  b = true;
}

I started debugging this code. When I stepped over the b = true statement the program crashed. This puzzled me. b is a local variable. It is stack allocated. How can an assignment to it fail?

#1: The memory monster - 2004, C#

I joined a small team working on a C# GUI app that was due to be released soon. We had a customer already using an early access version of the product in return for doing beta testing. The #1 item on our todo list was a report from this customer saying that the program becomes non-responsive after running for several hours. This is a serious defect, a real show stopper. As you can imagine, we never managed to reproduce the problem on our machines.

The release date got nearer and we still had no clue regarding the cause of this mysterious defect. As we had no better thing to do, we kept working on other items from our todo list, which was quite pathetic as we knew we will not be able to release the software with this defect.

At some point I decided to start fresh. I made the assumption that the defect was some sort of a leak.

Side note: Programmers often believe that in a garbage-collected environment memory leaks cannot occur. That's not true. A garbage collector (GC) will find all unreachable objects and will reclaim as many of them as possible. This does not mean that it will reclaim all unreachable objects. Many GC algorithms leave some of the garbage floating around for the next collection cycle. Moreover, a GC will consider something as garbage only if it is no longer reachable from your code. Thus, if your program maintains references to objects that are no longer needed, these objects will be considered, by the GC, as non-garbage. This will turn the program into a memory-consuming monster.

Such a leak often happens if you have some (software) cache in your code. The cache will keep references to objects - thereby preventing them from being collected - even if the application code no longer references them. Thus, if you implement a cache you must always implement some cleanup strategy.

I left the program running on my machine over the weekend hoping it will help me spot the leak. Sadly, when I came back to check on it, it was running smoothly. Disappointed, I sat down with the customer's contact person trying to understand how the program is being used. This conversation made me realize that the #1 thing that they (beta users) are doing much more than us (developers) is - wait for it - scrolling.

Ctrl+Alt+Delete -> Task Manager. I fired up the app, opened a data file, grabbed the scrollbar knob and started dragging it up and down. Looking at the Task Manager window I could see the Mem Usage value climbing. Slowly, but steadily. After a few minutes memory usage exceeded the main memory, the operating system started swapping and the program practically halted. This was awesome. I managed to reproduce the bug.

I opened the code that handled scrolling events (this was a custom widget with a custom data model that we developed). My eyes zeroed in on this loop:

for(int i = 0; i < rows.Length; ++i)
  if(rows[i].isOutOfView)
    rows.Remove(i);

Got it? Great.
Otherwise, wait for the next post...

(To be concluded)

Hackernews discussion