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;
}
}

8 comments :: So You Want to Practice your Code Reviewing Skills?

  1. A quick scan brought up the following:
    - the counter might roll over. I didn't check the match, but I'd guess for a 7 year old web app this is perfectly possible.
    - The singleton fails to stay a singleton when multiple jvm or classloaders are used. Again perfectly possible for a web application.
    - And i consider constructs like this:
    catch(Exception e) {
    sLogger.error("", e);
    }
    to be a bug.

  2. Lazy instantiation of the singleton instance is broken

    public static SiteInfo instance()
    is not synchronized and therefore it may occur that multiple threads get different instances of SiteInfo.

    The result is that a few session are not being counted, e.g. counted in the wrong SiteInfo instance.

  3. Reading first dozen of lines reveals that the singleton isn't thread safe: to be lazy it should either use DCL or Holder. In addition sessionCounter is neither volotile, nor guarded by syncronization when read in visitorCount(). Since there are synchronized methods I assume the class is intended to be used in multi-threaded environment, so probably a bug.

  4. This comment has been removed by the author.
  5. Other than the singleton instantiation synchronization problem (though visitorCount() itself should be ok, since simply returning an int is atomic IIRC).

    The code seems to initialize with the counter value of 100 instead of zero (INIT constant).

    Also, get() method seems problematic - it might return the empty string ("") if no rows are in the table, and the singleton only expects a valid number or null, which will cause a NumberFormatException

  6. - In many cases the DB resources aren't being closed.
    - The DB change isn't being committed so it has to rely on the underlying driver's behavior.
    - The insert statement doesn't explicitly defines column names, which is susceptible to failure if the order of rows in the DB table changes.
    - The update statement updates the row with param = 'SESSION_COUNT' although the insert inserts 'session_count'. Usually this would not work without special set-up in the db and/or connection.

    Not necessarily bugs, just bad coding:
    - The Map generated from the DB rows should be .
    - The logic of converting the ResultSet to a List of Maps is overcomplicated for the situation at hand, and could be dealt with more gracefully.
    - Anyway this DB Helper is used only here so it can be just a simple method, since the inner class is not static and the logic is very tailored to this class.

  7. As already said, bad attempt to make a singleton :

    Creation of SiteInfo is not synchronized, so you can have several instance at run time.

    Because it is not synchronized, the real value of "inst" may never be written to main memory from the CPU cache. So in theory you may have up to one instance of SiteInfo per CPU (but very unlikely).

    Application cannot be clustered :

    If there is several instance of the webapp running, not all values will be counted because several counter will exist and not be synchronized from JVM to JVM.

    Application does not deal with update/restart cases :

    Just imagine that the server is rebooted, or the application restarted after a code source update. Because counter is not updated each time, you'll may miss some hits.


    A side note : Just use one SQL statement incrementing a sequence is enough for what you need but will request the database more.

  8. I have read your blog its very attractive and impressive. I like it your blog.

    Java Training in Chennai Java Training in Chennai | Core Java Training in Chennai

    Online Java Training Online Java Training | Java J2EE Online Training | JavaEE Training Institute in Chennai m

Post a Comment