Deadlock accessing the database - don't use singleton StorIO in multiple threads

[tadas-subonis]

So after some time I was able to track the issue to these components:

DefaultStoreIOSQLite.java

   @Override
        public void beginTransaction() {
            synchronized (lock) {
                sqLiteOpenHelper
                        .getWritableDatabase()
                        .beginTransaction();

                numberOfRunningTransactions++;
            }
        }

and

SQLiteConnectionPool.java with SQLiteDatabase and SQLiteSession

    public SQLiteConnection acquireConnection(String sql, int connectionFlags,
            CancellationSignal cancellationSignal) {
        return waitForConnection(sql, connectionFlags, cancellationSignal);
    }

    private final ThreadLocal<SQLiteSession> mThreadSession = new ThreadLocal<SQLiteSession>() {
        @Override
        protected SQLiteSession initialValue() {
            return createSession();
        }
    };

The problem is that SQLiteSession takes a connection from SQLiteConnectionPool for the duration of the transaction and keeps it in ThreadLocal variable.

So lets assume we are doing something in Thread1 and start a transaction. We take a lock in DefaultStoreIOSQLite with "synchronized (lock)" and proceed - the connection is taken from the pool and no connections are left there. The "synchronized (lock)" is released.

Next a Thread2 comes and takes a lock "synchronized (lock)" and tries to take a connection by using "sqLiteOpenHelper.getWritableDatabase().beginTransaction();". Since there are no connections is starts to wait using "acquireConnection".

Then the Thread1 comes back with the work finished and tries to do "setTransactionSuccessful". "synchronized (lock)" is taken so he cannot do anything. "endTransaction" is never called and the connection that was taken from the pool is never available again. Thread2 is locked and Thread1 is locked.

Deadlock.

IMO, it would be the best to remove your lock "synchronized (lock)" as the underlying SQLiteDatabase takes care of proper connection sharing by itself.

Thanks

[artem-zinnatullin]

Good investigation, I tried to make deadlock with different implementations of DefaultStorIOSQLite before release 1.0.0 and current impl was LGTM, but your explanation makes sense.

I'll take a look at this tonight and provide solution, probably it will be possible to make it non blocking.

Thanks again!

[artem-zinnatullin]

I remembered why I decided to use synchronization instead of AtomicInteger and newSetFromMap(new ConcurrentHashMap()) for notification about changes: ConcurrentHashMap does not guarantee that if Thread1 is currently reading and removing items and Thread2 is adding items — Thread1 "may see, or not see" items that were added by Thread2 and if it will see them, there are no guarantees that when Thread2 will decide to iterate and remove items later — it may see items that were removed by Thread1, so we can send duplicate notifications.

But looks like I missed that synchronization over SQLiteDatabase transactions may cause deadlock. Thanks for investigation!

[artem-zinnatullin]

@tadas-subonis PTAL at #482

[artem-zinnatullin]

Resolved by #482.

[artem-zinnatullin]

1.1.2 should be available in 10-20 minutes!