SQLiteOpenHelper, worse than Hitler?

First a disclaimer, everything below is based on Android 2.3.3, maybe things have got better since then.

Second another disclaimer.  SQLiteOpenHelper is not worse than Hitler.  It is, in reality, only trying to help.  It is, I guess, more like a cat bringing you a dead pigeon.  It really thinks it is doing the right thing.  Stupid cats.  Let’s reset then

SQLiteOpenHelper, better than Hitler, worse than cats?

So the App we’ve currently got in development has had an irritating bug for quite some time. A background service runs every minute or two, scoops up some data and pings it off to a web server for processing. Every now and again we’d get an exception like the one below


10-29 23:48:24.772 14579 15293 V XxxxXxxDao: Opening Db for get
10-29 23:48:24.772 14579 15293 V XxxxXxxDao: Opened
10-29 23:48:25.374 14579 15293 V XxxxXxxDao: Closing Db for get
10-29 23:48:25.381 14579 15292 V XxxDao : Closing db for delete
10-29 23:48:25.381 14579 15292 W dalvikvm: threadid=38: thread exiting with uncaught exception (group=0x40018560)
10-29 23:48:25.389 14579 15293 V XxxxXxxDao: Opening Db for updateXxxxXxxData
10-29 23:48:25.405 14579 15292 E UncaughtException: restarting....
10-29 23:48:25.405 14579 15292 E UncaughtException: java.lang.IllegalStateException: database not open
10-29 23:48:25.405 14579 15292 E UncaughtException: at android.database.sqlite.SQLiteDatabase.delete(SQLiteDatabase.java:1628)
10-29 23:48:25.405 14579 15292 E UncaughtException: at x.xxxx.handheld.data.XxxDao.delete(XxxDao.java:117)
10-29 23:48:25.405 14579 15292 E UncaughtException: at x.xxxx.handheld.services.BackOfficeOperationsService.UploadXxxData(BackOfficeOperationsService.java:45)
10-29 23:48:25.405 14579 15292 E UncaughtException: at x.xxxx.handheld.services.BackOfficeSyncService$BackOfficeSync.run(BackOfficeSyncService.java:48)
10-29 23:48:25.405 14579 15292 E UncaughtException: at java.lang.Thread.run(Thread.java:1019)

A database two database objects would end up trying to access the database at the same time, one would find the database was closed and exception would be thrown.

Now this would seem like a pretty simple problem to solve. Except…well everything I could find talked about how using SQLiteOpenHelper was the solution, when you needed a new database object you called

database = sqliteOpenHelper.getWritableDatabase();

And then when you wanted to close it again you

sqliteOpenHelper.close();

It does all the hardwork of managing connections for you. It’s easy they said, it’s thread safe they said. Ha

 One does not simply open a database connection | Does not simply walk into mordor Boromir

Being good developers we had two methods in a base class, Open() and Close();

Open() does this

database = sqliteOpenHelper.getWritableDatabase();

and Close() does this

sqliteOpenHelper.close();

And your everyday get method looks something like this.

public TableObject get(String id) {
try {
Log.d(TAG,id);
TableObject tableObject = null;
Log.v(TAG, "Opening database for get");
open();
Log.v(TAG, "Opened");
String whereClause = DatabaseManager.TABLE_OBJECT_ID_COLUMN + "=" + id ;
Cursor cursor = database.query(DatabaseManager.TABLE_NAME,DatabaseManager.ALL_COLUMNS,whereClause,null,null,null,null);
while (cursor.moveToNext()) {
contravention = cursorToItem(cursor);
}
cursor.close();
return tableObject;
}

finally {
Log.v(TAG, “Closing database for get”);
close();
}

}

Everything always opens and always closes. Great.

So I hunted high and low for any cursor or db that was opened but not closed, but no sight of anything. All there was, was every now and again everything would crash to a halt, I’d read out the logcat and there would be the same db error. Taunting me.

But then today, Breakthrough! Some refactoring moved these db operations outside of the service and into a couple of AsyncTasks that ran simultaneously. And there was that very same error. Except this time it happened regularly at the same place in the app. It was time to fire up the debuggers, unleash the logging and work out what the hell was going on.

Turns out SQLiteOpenHelper is very stingy with its db connections. If there is one going around unused it’ll just pass you a reference to that one, rather than generate you a brand new one. The piece of logging that finally nailed this down looked like this

Log.v(tag, "database opened is " + database.hashCode());

Suddenly it all becomes clear.

Thread one is finishing up its database operation and going into close(), thread two is just starting and going into open().
Thread one is assigned thread two’s database connection since it is not in use but has not yet started using it
Thread two calls SQLiteOpenHelper.close() which closes all unused database connections including the one just assigned to thread one
Thread one tries to do a database operation and causes everything to blow up

Gives you a database connection closes it straight away | Scumbag Steve

So, problem, at long last, identified. SqliteOpenHelper is terrible.

What to do about it? Well we’re going to have to manage things ourselves. The solution is a counter that monitors how many database connections are open, and only closes when there is only one out there. The implementation is very simple – but honestly, we shouldn’t have to do this.

private static Integer counter = 0;
protected void open() throws SQLException {
synchronized (counter){
counter++;
database = sqliteOpenHelper.getWritableDatabase();
}
}

protected void close() throws SQLException {
Log.v(tag, “closing db – currently ” + (database == null ? “null” : database.isOpen() ? “open” : “closed”));
synchronized (counter){
if (counter.intValue() == 1)
sliteOpenHelper.close();
counter–;
}
}

And finally the evil bug is dead.

For some reason I couldn’t find anyone else writing about this problem online. So maybe there is something simple we missed?