From d518e94adbf2555b603bee5f2aa628f82cf1fdd9 Mon Sep 17 00:00:00 2001 From: stephan Date: Mon, 14 Aug 2023 08:28:46 +0000 Subject: [PATCH] JNI-internal docs and removal of obsolete code. FossilOrigin-Name: b62d93258b6a661f3a9b61468b3b641c14faf2d2196f78aca95fe14de43c9444 --- ext/jni/src/c/sqlite3-jni.c | 81 ++++++++++++++++--------- ext/jni/src/org/sqlite/jni/Tester1.java | 9 ++- manifest | 14 ++--- manifest.uuid | 2 +- 4 files changed, 70 insertions(+), 36 deletions(-) diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c index 55ecd4b2dc..129a601210 100644 --- a/ext/jni/src/c/sqlite3-jni.c +++ b/ext/jni/src/c/sqlite3-jni.c @@ -432,9 +432,11 @@ struct S3JniHook{ jmethodID midCallback /* callback method. Signature depends on jObj's type */; jclass klazz /* global ref to jObj's class. Only needed - by hooks which have an xDestroy() method, - as lookup of that method is deferred - until the object requires cleanup. */; + by hooks which have an xDestroy() method. + We can probably eliminate this and simply + do the class lookup at the same + (deferred) time we do the xDestroy() + lookup. */; }; /** @@ -522,19 +524,38 @@ static struct { unsigned nInverse; } udf; } metrics; + /** + The list of bound auto-extensions (Java-side: + org.sqlite.jni.AutoExtension objects). Because this data + structure cannot be manipulated during traversal (without adding + more code to deal with it), and to avoid a small window where a + call to sqlite3_reset/clear_auto_extension() could lock the + structure during an open() call, we lock this mutex before + sqlite3_open() is called and unlock it once sqlite3_open() + returns. + */ struct { S3JniAutoExtension *pHead /* Head of the auto-extension list */; - S3JniDb * pdbOpening /* FIXME: move into envCache. Handle to the - being-opened db. We need this so that auto - extensions can have a consistent view of - the cross-language db connection and - behave property if they call further db - APIs. */; - int isRunning /* True while auto extensions are - running. This is used to prohibit - manipulation of the auto-extension - list while extensions are - running. */; + /** + pdbOpening is used to coordinate the Java/DB connection of a + being-open()'d db. "The problem" is that auto-extensions run + before we can bind the C db to its Java representation, but + auto-extensions require that binding. We handle this as + follows: + + - At the start of open(), we lock on this->mutex. + - Allocate the Java side of that connection and set pdbOpening + to point to that object. + - Call open(), which triggers the auto-extension handler. + That handler uses pdbOpening to connect the native db handle + which it recieves with pdbOpening. + - Return from open(). + - Clean up and unlock the mutex. + + If open() did not block on a mutex, there would be a race + condition in which two open() calls could set pdbOpening. + */ + S3JniDb * pdbOpening; sqlite3_mutex * mutex /* mutex for aUsed and aFree */; void const * locker /* Mutex is locked on this object's behalf */; } autoExt; @@ -1906,7 +1927,6 @@ static int s3jni_run_java_auto_extensions(sqlite3 *pDb, const char **pzErr, assert( S3JniGlobal.autoExt.locker == ps ); S3JniGlobal.autoExt.pdbOpening = 0; if( !pAX ){ - assert( 0==S3JniGlobal.autoExt.isRunning ); return 0; }else if( S3JniGlobal.autoExt.locker != ps ) { *pzErr = sqlite3_mprintf("Internal error: unexpected path lead to " @@ -1919,7 +1939,6 @@ static int s3jni_run_java_auto_extensions(sqlite3 *pDb, const char **pzErr, ps->pDb = pDb; assert( ps->jDb ); NativePointerHolder_set(env, ps->jDb, pDb, S3JniClassNames.sqlite3); - ++S3JniGlobal.autoExt.isRunning; for( ; pAX; pAX = pAX->pNext ){ rc = (*env)->CallIntMethod(env, pAX->jObj, pAX->midFunc, ps->jDb); IFTHREW { @@ -1936,7 +1955,6 @@ static int s3jni_run_java_auto_extensions(sqlite3 *pDb, const char **pzErr, break; } } - --S3JniGlobal.autoExt.isRunning; return rc; } @@ -2100,13 +2118,11 @@ JDECL(jboolean,1cancel_1auto_1extension)(JENV_CSELF, jobject jAutoExt){ S3JniAutoExtension * ax; jboolean rc = JNI_FALSE; MUTEX_ENTER_EXT; - if( !S3JniGlobal.autoExt.isRunning ) { - for( ax = S3JniGlobal.autoExt.pHead; ax; ax = ax->pNext ){ - if( (*env)->IsSameObject(env, ax->jObj, jAutoExt) ){ - S3JniAutoExtension_free(env, ax); - rc = JNI_TRUE; - break; - } + for( ax = S3JniGlobal.autoExt.pHead; ax; ax = ax->pNext ){ + if( (*env)->IsSameObject(env, ax->jObj, jAutoExt) ){ + S3JniAutoExtension_free(env, ax); + rc = JNI_TRUE; + break; } } MUTEX_LEAVE_EXT; @@ -2660,7 +2676,14 @@ static int s3jni_open_pre(JNIEnv * const env, S3JniEnv **jc, jstring jDbName, char **zDbName, S3JniDb ** ps, jobject *jDb){ int rc = 0; - MUTEX_TRY_EXT(return SQLITE_BUSY); + MUTEX_TRY_EXT(return SQLITE_BUSY) + /* we don't wait forever here because it could lead to a deadlock + if an auto-extension opens a database. Without a mutex, that + situation leads to infinite recursion and stack overflow, which + is infinitely easier to track down from client code. Note that + we rely on the Java methods for open() and auto-extension + handling to be synchronized so that this BUSY cannot be + triggered by a race condition with those functions. */; *jc = S3JniGlobal_env_cache(env); if(!*jc){ rc = SQLITE_NOMEM; @@ -2714,8 +2737,12 @@ static int s3jni_open_post(JNIEnv * const env, S3JniDb * ps, S3JniGlobal.autoExt.pdbOpening = 0; if(*ppDb){ assert(ps->jDb); - ps->pDb = *ppDb; - NativePointerHolder_set(env, ps->jDb, *ppDb, S3JniClassNames.sqlite3); + if( 0==ps->pDb ){ + ps->pDb = *ppDb; + NativePointerHolder_set(env, ps->jDb, *ppDb, S3JniClassNames.sqlite3); + }else{ + assert( ps->pDb == *ppDb /* set up via s3jni_run_java_auto_extensions() */); + } }else{ MUTEX_ENTER_PDB; S3JniDb_set_aside(ps); diff --git a/ext/jni/src/org/sqlite/jni/Tester1.java b/ext/jni/src/org/sqlite/jni/Tester1.java index e9524e49a7..055e070bfd 100644 --- a/ext/jni/src/org/sqlite/jni/Tester1.java +++ b/ext/jni/src/org/sqlite/jni/Tester1.java @@ -1079,10 +1079,17 @@ public class Tester1 { affirm( 0==rc ); sqlite3_close( createNewDb() ); affirm( 3==val.value ); + + sqlite3 db = createNewDb(); + affirm( 4==val.value ); + execSql(db, "ATTACH ':memory' as foo"); + affirm( 4==val.value /* ATTACH uses the same connection, not sub-connections. */ ); + sqlite3_close(db); + affirm( sqlite3_cancel_auto_extension(ax) ); affirm( !sqlite3_cancel_auto_extension(ax) ); sqlite3_close(createNewDb()); - affirm( 3==val.value ); + affirm( 4==val.value ); rc = sqlite3_auto_extension( ax ); affirm( 0==rc ); Exception err = null; diff --git a/manifest b/manifest index ad2950dc49..8673db6eeb 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Internal\sAPI\srenaming\sfor\sclarity's\ssake. -D 2023-08-13T20:58:12.729 +C JNI-internal\sdocs\sand\sremoval\sof\sobsolete\scode. +D 2023-08-14T08:28:46.677 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -234,7 +234,7 @@ F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a3 F ext/jni/GNUmakefile a9e11b92e620058558cbc1a2d49f8ec53c78d6a989b9db0b7d0b649b9f174881 F ext/jni/README.md 7a614a2fa6c561205f7a53fd8626cf93a7b5711ff454fc1814517f796df398eb F ext/jni/jar-dist.make f90a553203a57934bf275bed86479485135a52f48ac5c1cfe6499ae07b0b35a4 -F ext/jni/src/c/sqlite3-jni.c 1b1f3fa286476933171e75465214deab44c55714aaaa80b5ce145e89f10b0df8 +F ext/jni/src/c/sqlite3-jni.c 1bc1cbaf075065793f83cf5bfba631216ced48d7beae0768ebd838e923e7e590 F ext/jni/src/c/sqlite3-jni.h f10d2f38720687c70ecdd5e44f6e8db98efee2caa05fc86b2d9e0c76e6cc0a18 F ext/jni/src/org/sqlite/jni/Authorizer.java 1308988f7f40579ea0e4deeaec3c6be971630566bd021c31367fe3f5140db892 F ext/jni/src/org/sqlite/jni/AutoExtension.java 18e83f6f463e306df60b2dceb65247d32af1f78af4bbbae9155411a8c6cdb093 @@ -255,7 +255,7 @@ F ext/jni/src/org/sqlite/jni/ResultCode.java ba701f20213a5f259e94cfbfdd36eb7ac7c F ext/jni/src/org/sqlite/jni/RollbackHook.java b04c8abcc6ade44a8a57129e33765793f69df0ba909e49ba18d73f4268d92564 F ext/jni/src/org/sqlite/jni/SQLFunction.java 09ce81c1c637e31c3a830d4c859cce95d65f5e02ff45f8bd1985b3479381bc46 F ext/jni/src/org/sqlite/jni/SQLite3Jni.java 5eeba0b1a00fb34bc93fe60186f6032fcf4d568fc5868d70029883d3d07cc306 -F ext/jni/src/org/sqlite/jni/Tester1.java fc2ec1f1be58474112b9df8284f0157b64872107f446154c3d0bf1742b924d2b +F ext/jni/src/org/sqlite/jni/Tester1.java 368e836d943d9e882d2a217d0f582ed4141d164f174bebc50715acd57549a09b F ext/jni/src/org/sqlite/jni/TesterFts5.java 59e22dd24af033ea8827d36225a2f3297908fb6af8818ead8850c6c6847557b1 F ext/jni/src/org/sqlite/jni/Tracer.java a5cece9f947b0af27669b8baec300b6dd7ff859c3e6a6e4a1bd8b50f9714775d F ext/jni/src/org/sqlite/jni/UpdateHook.java e58645a1727f8a9bbe72dc072ec5b40d9f9362cb0aa24acfe93f49ff56a9016d @@ -2091,8 +2091,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P 8da97e0db4eeacf91aa6fd909fd7cb73b050d194dfc7739a502b55f7eca6d7b1 -R 582810911695a14eb1f701cb46e67a37 +P 911e4fc5aaf9478214095a65f74af3ebca883922c36cf7a8d911116c42cf9de8 +R c6d49900ed87c14449d9c08a36536c71 U stephan -Z 414dea18ee7e2187dd201f9308cbe98b +Z 09908deaf1ed6a1fae2d9bef97861a7c # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 6ade59ea97..7a0d99b2d3 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -911e4fc5aaf9478214095a65f74af3ebca883922c36cf7a8d911116c42cf9de8 \ No newline at end of file +b62d93258b6a661f3a9b61468b3b641c14faf2d2196f78aca95fe14de43c9444 \ No newline at end of file