diff --git a/core/java/android/database/sqlite/SQLiteCompiledSql.java b/core/java/android/database/sqlite/SQLiteCompiledSql.java index 16ff2ab6e3e95..9889a21ed7001 100644 --- a/core/java/android/database/sqlite/SQLiteCompiledSql.java +++ b/core/java/android/database/sqlite/SQLiteCompiledSql.java @@ -95,13 +95,8 @@ import android.util.Log; if (SQLiteDebug.DEBUG_ACTIVE_CURSOR_FINALIZATION) { Log.v(TAG, "closed and deallocated DbObj (id#" + nStatement +")"); } - try { - mDatabase.lock(); - native_finalize(); - nStatement = 0; - } finally { - mDatabase.unlock(); - } + mDatabase.finalizeStatementLater(nStatement); + nStatement = 0; } } @@ -159,5 +154,4 @@ import android.util.Log; * @param sql The SQL to compile. */ private final native void native_compile(String sql); - private final native void native_finalize(); } diff --git a/core/java/android/database/sqlite/SQLiteDatabase.java b/core/java/android/database/sqlite/SQLiteDatabase.java index 47de2868fc33b..e6f54e263c3f6 100644 --- a/core/java/android/database/sqlite/SQLiteDatabase.java +++ b/core/java/android/database/sqlite/SQLiteDatabase.java @@ -230,8 +230,8 @@ public class SQLiteDatabase extends SQLiteClosable { // lock acquistions of the database. /* package */ static final String GET_LOCK_LOG_PREFIX = "GETLOCK:"; - /** Used by native code, do not rename */ - /* package */ int mNativeHandle = 0; + /** Used by native code, do not rename. make it volatile, so it is thread-safe. */ + /* package */ volatile int mNativeHandle = 0; /** Used to make temp table names unique */ /* package */ int mTempTableSequence = 0; @@ -313,6 +313,9 @@ public class SQLiteDatabase extends SQLiteClosable { private static final String LOG_SLOW_QUERIES_PROPERTY = "db.log.slow_query_threshold"; private final int mSlowQueryThreshold; + /** stores the list of statement ids that need to be finalized by sqlite */ + private ArrayList mClosedStatementIds = new ArrayList(); + /** {@link DatabaseErrorHandler} to be used when SQLite returns any of the following errors * Corruption * */ @@ -1778,6 +1781,7 @@ public class SQLiteDatabase extends SQLiteClosable { } logTimeStat(mLastSqlStatement, timeStart, GET_LOCK_LOG_PREFIX); try { + closePendingStatements(); native_execSQL(sql); } catch (SQLiteDatabaseCorruptException e) { onCorruption(); @@ -2101,6 +2105,52 @@ public class SQLiteDatabase extends SQLiteClosable { mMaxSqlCacheSize = cacheSize; } + /* package */ void finalizeStatementLater(int id) { + if (!isOpen()) { + // database already closed. this statement will already have been finalized. + return; + } + synchronized(mClosedStatementIds) { + if (mClosedStatementIds.contains(id)) { + // this statement id is already queued up for finalization. + return; + } + mClosedStatementIds.add(id); + } + } + + /** + * public visibility only for testing. otherwise, package visibility is sufficient + * @hide + */ + public void closePendingStatements() { + if (!isOpen()) { + // since this database is already closed, no need to finalize anything. + mClosedStatementIds.clear(); + return; + } + verifyLockOwner(); + /* to minimize synchronization on mClosedStatementIds, make a copy of the list */ + ArrayList list = new ArrayList(mClosedStatementIds.size()); + synchronized(mClosedStatementIds) { + list.addAll(mClosedStatementIds); + mClosedStatementIds.clear(); + } + // finalize all the statements from the copied list + int size = list.size(); + for (int i = 0; i < size; i++) { + native_finalize(list.get(i)); + } + } + + /** + * for testing only + * @hide + */ + public ArrayList getQueuedUpStmtList() { + return mClosedStatementIds; + } + static class ActiveDatabases { private static final ActiveDatabases activeDatabases = new ActiveDatabases(); private HashSet> mActiveDatabases = @@ -2310,4 +2360,11 @@ public class SQLiteDatabase extends SQLiteClosable { * @return int value of SQLITE_DBSTATUS_LOOKASIDE_USED */ private native int native_getDbLookaside(); + + /** + * finalizes the given statement id. + * + * @param statementId statement to be finzlied by sqlite + */ + private final native void native_finalize(int statementId); } diff --git a/core/java/android/database/sqlite/SQLiteDirectCursorDriver.java b/core/java/android/database/sqlite/SQLiteDirectCursorDriver.java index ac60b2706619e..be49257988151 100644 --- a/core/java/android/database/sqlite/SQLiteDirectCursorDriver.java +++ b/core/java/android/database/sqlite/SQLiteDirectCursorDriver.java @@ -43,6 +43,7 @@ public class SQLiteDirectCursorDriver implements SQLiteCursorDriver { try { mDatabase.lock(); + mDatabase.closePendingStatements(); query = new SQLiteQuery(mDatabase, mSql, 0, selectionArgs); // Arg binding int numArgs = selectionArgs == null ? 0 : selectionArgs.length; diff --git a/core/java/android/database/sqlite/SQLiteProgram.java b/core/java/android/database/sqlite/SQLiteProgram.java index c37385c0ed97a..dc1a0eef14554 100644 --- a/core/java/android/database/sqlite/SQLiteProgram.java +++ b/core/java/android/database/sqlite/SQLiteProgram.java @@ -291,12 +291,7 @@ public abstract class SQLiteProgram extends SQLiteClosable { if (!mDatabase.isOpen()) { return; } - mDatabase.lock(); - try { - releaseReference(); - } finally { - mDatabase.unlock(); - } + releaseReference(); } /** diff --git a/core/java/android/database/sqlite/SQLiteStatement.java b/core/java/android/database/sqlite/SQLiteStatement.java index 47cca873e91f7..ba0e63bd60998 100644 --- a/core/java/android/database/sqlite/SQLiteStatement.java +++ b/core/java/android/database/sqlite/SQLiteStatement.java @@ -55,6 +55,7 @@ public class SQLiteStatement extends SQLiteProgram acquireReference(); try { + mDatabase.closePendingStatements(); native_execute(); mDatabase.logTimeStat(mSql, timeStart); } finally { @@ -81,6 +82,7 @@ public class SQLiteStatement extends SQLiteProgram acquireReference(); try { + mDatabase.closePendingStatements(); native_execute(); mDatabase.logTimeStat(mSql, timeStart); return (mDatabase.lastChangeCount() > 0) ? mDatabase.lastInsertRow() : -1; @@ -107,6 +109,7 @@ public class SQLiteStatement extends SQLiteProgram acquireReference(); try { + mDatabase.closePendingStatements(); long retValue = native_1x1_long(); mDatabase.logTimeStat(mSql, timeStart); return retValue; @@ -133,6 +136,7 @@ public class SQLiteStatement extends SQLiteProgram acquireReference(); try { + mDatabase.closePendingStatements(); String retValue = native_1x1_string(); mDatabase.logTimeStat(mSql, timeStart); return retValue; diff --git a/core/jni/android_database_SQLiteCompiledSql.cpp b/core/jni/android_database_SQLiteCompiledSql.cpp index 8d1c39ee26ef3..de4c5c8097bf6 100644 --- a/core/jni/android_database_SQLiteCompiledSql.cpp +++ b/core/jni/android_database_SQLiteCompiledSql.cpp @@ -91,22 +91,11 @@ static void native_compile(JNIEnv* env, jobject object, jstring sqlString) compile(env, object, GET_HANDLE(env, object), sqlString); } -static void native_finalize(JNIEnv* env, jobject object) -{ - int err; - sqlite3_stmt * statement = GET_STATEMENT(env, object); - - if (statement != NULL) { - sqlite3_finalize(statement); - env->SetIntField(object, gStatementField, 0); - } -} static JNINativeMethod sMethods[] = { /* name, signature, funcPtr */ {"native_compile", "(Ljava/lang/String;)V", (void *)native_compile}, - {"native_finalize", "()V", (void *)native_finalize}, }; int register_android_database_SQLiteCompiledSql(JNIEnv * env) diff --git a/core/jni/android_database_SQLiteDatabase.cpp b/core/jni/android_database_SQLiteDatabase.cpp index 36234a96e0f63..3d2f54da8cd90 100644 --- a/core/jni/android_database_SQLiteDatabase.cpp +++ b/core/jni/android_database_SQLiteDatabase.cpp @@ -215,6 +215,7 @@ static void enableSqlProfiling(JNIEnv* env, jobject object, jstring databaseName static void dbclose(JNIEnv* env, jobject object) { sqlite3 * handle = (sqlite3 *)env->GetIntField(object, offset_db_handle); + sqlite3_stmt * pStmt; if (handle != NULL) { // release the memory associated with the traceFuncArg in enableSqlTracing function @@ -227,6 +228,10 @@ static void dbclose(JNIEnv* env, jobject object) if (traceFuncArg != NULL) { free(traceFuncArg); } + // finalize all statements on this handle + while ((pStmt = sqlite3_next_stmt(handle, 0)) != 0 ) { + sqlite3_finalize(pStmt); + } LOGV("Closing database: handle=%p\n", handle); int result = sqlite3_close(handle); if (result == SQLITE_OK) { @@ -443,6 +448,13 @@ static jint native_releaseMemory(JNIEnv *env, jobject clazz) return sqlite3_release_memory(SQLITE_SOFT_HEAP_LIMIT); } +static void native_finalize(JNIEnv* env, jobject object, jint statementId) +{ + if (statementId > 0) { + sqlite3_finalize((sqlite3_stmt *)statementId); + } +} + static JNINativeMethod sMethods[] = { /* name, signature, funcPtr */ @@ -456,6 +468,7 @@ static JNINativeMethod sMethods[] = {"native_setLocale", "(Ljava/lang/String;I)V", (void *)native_setLocale}, {"native_getDbLookaside", "()I", (void *)native_getDbLookaside}, {"releaseMemory", "()I", (void *)native_releaseMemory}, + {"native_finalize", "(I)V", (void *)native_finalize}, }; int register_android_database_SQLiteDatabase(JNIEnv *env) diff --git a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java index d9a234a3c0cee..c584398702fd7 100644 --- a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java +++ b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java @@ -1232,4 +1232,190 @@ public class DatabaseGeneralTest extends AndroidTestCase implements PerformanceT fail("unexpected"); } } + + /** + * test to make sure the statement finalizations are not done right away but + * piggybacked onto the next sql statement execution on the same database. + */ + @SmallTest + public void testStatementClose() { + mDatabase.execSQL("CREATE TABLE test (i int);"); + // fill up statement cache in mDatabase\ + int N = 26; + mDatabase.setMaxSqlCacheSize(N); + SQLiteStatement stmt; + int stmt0Id = 0; + for (int i = 0; i < N; i ++) { + stmt = mDatabase.compileStatement("insert into test values(" + i + ");"); + stmt.executeInsert(); + // keep track of 0th entry + if (i == 0) { + stmt0Id = stmt.getUniqueId(); + } + stmt.close(); + } + + // add one more to the cache - and the above 'stmt0Id' should fall out of cache + SQLiteStatement stmt1 = mDatabase.compileStatement("select * from test where i = 1;"); + stmt1.close(); + + // the above close() should have queuedUp the statement for finalization + ArrayList statementIds = mDatabase.getQueuedUpStmtList(); + assertTrue(statementIds.contains(stmt0Id)); + + // execute something to see if this statement gets finalized + mDatabase.execSQL("delete from test where i = 10;"); + statementIds = mDatabase.getQueuedUpStmtList(); + assertEquals(0, statementIds.size()); + } + + /** + * same as above - except that the statement to be finalized is from Thread # 1. + * and it is eventually finalized in Thread # 2 when it executes a sql statement. + * @throws InterruptedException + */ + @LargeTest + public void testStatementCloseDiffThread() throws InterruptedException { + mDatabase.execSQL("CREATE TABLE test (i int);"); + // fill up statement cache in mDatabase in a thread + Thread t1 = new Thread() { + @Override public void run() { + int N = 26; + mDatabase.setMaxSqlCacheSize(N); + SQLiteStatement stmt; + for (int i = 0; i < N; i ++) { + stmt = mDatabase.compileStatement("insert into test values(" + i + ");"); + stmt.executeInsert(); + // keep track of 0th entry + if (i == 0) { + setStmt0Id(stmt.getUniqueId()); + } + stmt.close(); + } + } + }; + t1.start(); + // wait for the thread to finish + t1.join(); + + // add one more to the cache - and the above 'stmt0Id' should fall out of cache + // just for the heck of it, do it in a separate thread + Thread t2 = new Thread() { + @Override public void run() { + SQLiteStatement stmt1 = mDatabase.compileStatement( + "select * from test where i = 1;"); + stmt1.close(); + } + }; + t2.start(); + t2.join(); + + // close() in the above thread should have queuedUp the statement for finalization + ArrayList statementIds = mDatabase.getQueuedUpStmtList(); + assertTrue(getStmt0Id() > 0); + assertTrue(statementIds.contains(stmt0Id)); + assertEquals(1, statementIds.size()); + + // execute something to see if this statement gets finalized + // again do it in a separate thread + Thread t3 = new Thread() { + @Override public void run() { + mDatabase.execSQL("delete from test where i = 10;"); + } + }; + t3.start(); + t3.join(); + + // is the statement finalized? + statementIds = mDatabase.getQueuedUpStmtList(); + assertEquals(0, statementIds.size()); + } + + private volatile int stmt0Id = 0; + private synchronized void setStmt0Id(int stmt0Id) { + this.stmt0Id = stmt0Id; + } + private synchronized int getStmt0Id() { + return this.stmt0Id; + } + + /** + * same as above - except that the queue of statements to be finalized are finalized + * by database close() operation. + */ + @LargeTest + public void testStatementCloseByDbClose() throws InterruptedException { + mDatabase.execSQL("CREATE TABLE test (i int);"); + // fill up statement cache in mDatabase in a thread + Thread t1 = new Thread() { + @Override public void run() { + int N = 26; + mDatabase.setMaxSqlCacheSize(N); + SQLiteStatement stmt; + for (int i = 0; i < N; i ++) { + stmt = mDatabase.compileStatement("insert into test values(" + i + ");"); + stmt.executeInsert(); + // keep track of 0th entry + if (i == 0) { + setStmt0Id(stmt.getUniqueId()); + } + stmt.close(); + } + } + }; + t1.start(); + // wait for the thread to finish + t1.join(); + + // add one more to the cache - and the above 'stmt0Id' should fall out of cache + // just for the heck of it, do it in a separate thread + Thread t2 = new Thread() { + @Override public void run() { + SQLiteStatement stmt1 = mDatabase.compileStatement( + "select * from test where i = 1;"); + stmt1.close(); + } + }; + t2.start(); + t2.join(); + + // close() in the above thread should have queuedUp the statement for finalization + ArrayList statementIds = mDatabase.getQueuedUpStmtList(); + assertTrue(getStmt0Id() > 0); + assertTrue(statementIds.contains(stmt0Id)); + assertEquals(1, statementIds.size()); + + // close the database. native method dbclose() will finalize all statements + // before closing the database. + // again do it in a separate thread + Thread t3 = new Thread() { + @Override public void run() { + mDatabase.close(); + } + }; + t3.start(); + t3.join(); + + // check mClosedStatementIds in mDatabase. it should still have 'stmt0Id' + statementIds = mDatabase.getQueuedUpStmtList(); + assertTrue(statementIds.contains(stmt0Id)); + + // try to finalize the pending statements. and there should be no exceptions from anywhere + // just for the heck of it, do it in a separate thread + Thread t4 = new Thread() { + @Override public void run() { + try { + mDatabase.closePendingStatements(); + } catch (Exception e) { + fail("not expected"); + } + } + }; + t4.start(); + t4.join(); + + // mClosedStatementIds in mDatabase should be empty + statementIds = mDatabase.getQueuedUpStmtList(); + assertEquals(0, statementIds.size()); + } }