From: stephan <stephan@noemail.net>
Date: Mon, 31 Jul 2023 10:08:36 +0000 (+0000)
Subject: Refactor the busy-handler-specific JNI hook type to use the generic hook type.
X-Git-Tag: version-3.43.0~47^2~119
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=452108b4fc0efc688c86b41e3413b211f817d59c;p=thirdparty%2Fsqlite.git

Refactor the busy-handler-specific JNI hook type to use the generic hook type.

FossilOrigin-Name: d9efdc6dd20a34bfdaad5d4bf8e67cce7e35238299eb91e4459d59fda11978a6
---

diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c
index b6e19a10a7..454dacb55f 100644
--- a/ext/jni/src/c/sqlite3-jni.c
+++ b/ext/jni/src/c/sqlite3-jni.c
@@ -336,24 +336,14 @@ static void JNIEnvCache_clear(JNIEnvCache * p){
 typedef struct JniHookState JniHookState;
 struct JniHookState{
   jobject jObj            /* global ref to Java instance */;
-  jmethodID midCallback   /* callback method */;
-  jclass klazz            /* jObj's class. Not needed by all types. */;
+  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. */;
 };
 
-/**
-   State for binding Java-side callbacks which potentially have an
-   xDestroy() method.  Maintenance reminder: this is different from
-   JniHookState because of the need to look up the finalizer. TODO:
-   look into consolidating this with JniHookState, perhaps adding the
-   jclass member to that object.
-*/
-typedef struct BusyHandlerJni BusyHandlerJni;
-struct BusyHandlerJni{
-  JniHookState base;
-  jclass klazz          /* jObj's class */;
-};
-
-
 /**
    Per-(sqlite3*) state for bindings which do not have their own
    finalizer functions, e.g. tracing and commit/rollback hooks.  This
@@ -380,14 +370,14 @@ struct PerDbStateJni {
                  receive. */;
   PerDbStateJni * pNext /* Next entry in the available/free list */;
   PerDbStateJni * pPrev /* Previous entry in the available/free list */;
-  JniHookState trace;
-  JniHookState progress;
+  JniHookState busyHandler;
+  JniHookState collation;
+  JniHookState collationNeeded;
   JniHookState commitHook;
+  JniHookState progress;
   JniHookState rollbackHook;
+  JniHookState trace;
   JniHookState updateHook;
-  JniHookState collation;
-  JniHookState collationNeeded;
-  BusyHandlerJni busyHandler;
 };
 
 static struct {
@@ -457,21 +447,21 @@ static int s3jni_db_error(sqlite3*db, int err_code, const char *zMsg){
 
 /**
    Extracts the (void xDestroy()) method from the given jclass and
-   applies it to jobj. If jobs is NULL, this is a no-op. If klazz is
+   applies it to jobj. If jObj is NULL, this is a no-op. If klazz is
    NULL then it's derived from jobj. The lack of an xDestroy() method
    is silently ignored and any exceptions thrown by the method trigger
    a warning to stdout or stderr and then the exception is suppressed.
 */
-static void s3jni_call_xDestroy(JNIEnv *env, jobject jobj, jclass klazz){
-  if(jobj){
+static void s3jni_call_xDestroy(JNIEnv *env, jobject jObj, jclass klazz){
+  if(jObj){
     jmethodID method;
     if(!klazz){
-      klazz = (*env)->GetObjectClass(env, jobj);
+      klazz = (*env)->GetObjectClass(env, jObj);
       assert(klazz);
     }
     method = (*env)->GetMethodID(env, klazz, "xDestroy", "()V");
     if(method){
-      (*env)->CallVoidMethod(env, jobj, method);
+      (*env)->CallVoidMethod(env, jObj, method);
       IFTHREW{
         EXCEPTION_WARN_CALLBACK_THREW;
         EXCEPTION_CLEAR;
@@ -482,41 +472,6 @@ static void s3jni_call_xDestroy(JNIEnv *env, jobject jobj, jclass klazz){
   }
 }
 
-
-/**
-   Clears s's state, releasing any Java references. Before doing so,
-   it calls s's xDestroy() method, ignoring the lack of that method or
-   any exceptions it throws. This is a no-op of s has no current
-   state.
-*/
-static void BusyHandlerJni_clear(JNIEnv *env, BusyHandlerJni * const s){
-  if(s->base.jObj){
-    s3jni_call_xDestroy(env, s->base.jObj, s->klazz);
-    UNREF_G(s->base.jObj);
-    UNREF_G(s->klazz);
-    memset(s, 0, sizeof(BusyHandlerJni));
-  }
-}
-
-/**
-   Initializes s to wrap BusyHandlerJni-type object jObject, clearing
-   any current state of s beforehand. Returns 0 on success, non-0 on
-   error. On error, s's state is cleared.
-*/
-static int BusyHandlerJni_init(JNIEnv * const env, BusyHandlerJni * const s,
-                               jobject jObj){
-  const char * zSig = "(I)I" /* callback signature */;
-  if(s->base.jObj) BusyHandlerJni_clear(env, s);
-  s->base.jObj = REF_G(jObj);
-  s->klazz = REF_G((*env)->GetObjectClass(env, jObj));
-  s->base.midCallback = (*env)->GetMethodID(env, s->klazz, "xCallback", zSig);
-  IFTHREW {
-    BusyHandlerJni_clear(env, s);
-    return SQLITE_ERROR;
-  }
-  return 0;
-}
-
 /**
    Fetches the S3Global.envCache row for the given env, allocing
    a row if needed. When a row is allocated, its state is initialized
@@ -727,13 +682,22 @@ static PerDbStateJni * PerDbStateJni_alloc(JNIEnv *env, sqlite3 *pDb, jobject jD
   return rv;
 }
 
+/**
+   Removes any Java references from s and clears its state. If
+   doXDestroy is true and s->klazz and s->jObj are not NULL, s->jObj's
+   s is passed to s3jni_call_xDestroy() before any references are
+   cleared. It is legal to call this when the object has no Java
+   references.
+*/
 static void JniHookState_unref(JNIEnv * const env, JniHookState * const s, int doXDestroy){
   if(doXDestroy && s->klazz && s->jObj){
     s3jni_call_xDestroy(env, s->jObj, s->klazz);
   }
   UNREF_G(s->jObj);
   UNREF_G(s->klazz);
-  memset(s, 0, sizeof(JniHookState));
+  s->jObj = 0;
+  s->klazz = 0;
+  s->midCallback = 0;
 }
 
 /**
@@ -762,9 +726,9 @@ static void PerDbStateJni_set_aside(PerDbStateJni * const s){
     UNHOOK(updateHook, 0);
     UNHOOK(collation, 1);
     UNHOOK(collationNeeded, 1);
+    UNHOOK(busyHandler, 1);
 #undef UNHOOK
     UNREF_G(s->jDb);
-    BusyHandlerJni_clear(env, &s->busyHandler);
     memset(s, 0, sizeof(PerDbStateJni));
     s->pNext = S3Global.perDb.aFree;
     if(s->pNext) s->pNext->pPrev = s;
@@ -781,7 +745,7 @@ static void PerDbStateJni_dump(PerDbStateJni *s){
   MARKER(("PerDbStateJni->progress.jObj @ %p\n", s->progress.jObj));
   MARKER(("PerDbStateJni->commitHook.jObj @ %p\n", s->commitHook.jObj));
   MARKER(("PerDbStateJni->rollbackHook.jObj @ %p\n", s->rollbackHook.jObj));
-  MARKER(("PerDbStateJni->busyHandler.jObj @ %p\n", s->busyHandler.base.jObj));
+  MARKER(("PerDbStateJni->busyHandler.jObj @ %p\n", s->busyHandler.jObj));
   MARKER(("PerDbStateJni->env @ %p\n", s->env));
 }
 
@@ -1394,34 +1358,42 @@ JDECL(jint,1bind_1zeroblob64)(JENV_JSELF, jobject jpStmt,
 static int s3jni_busy_handler(void* pState, int n){
   PerDbStateJni * const ps = (PerDbStateJni *)pState;
   int rc = 0;
-  if( ps->busyHandler.base.jObj ){
+  if( ps->busyHandler.jObj ){
     JNIEnv * const env = ps->env;
-    rc = (*env)->CallIntMethod(env, ps->busyHandler.base.jObj,
-                               ps->busyHandler.base.midCallback, (jint)n);
-    IFTHREW_CLEAR;
+    rc = (*env)->CallIntMethod(env, ps->busyHandler.jObj,
+                               ps->busyHandler.midCallback, (jint)n);
+    IFTHREW{
+      EXCEPTION_WARN_CALLBACK_THREW;
+      EXCEPTION_CLEAR;
+      rc = s3jni_db_error(ps->pDb, SQLITE_ERROR, "busy-handle callback threw.");
+    }
   }
   return rc;
 }
 
 JDECL(jint,1busy_1handler)(JENV_JSELF, jobject jDb, jobject jBusy){
   PerDbStateJni * const ps = PerDbStateJni_for_db(env, jDb, 0, 0);
-  int rc;
+  int rc = 0;
   if(!ps) return (jint)SQLITE_NOMEM;
   if(jBusy){
-    if(ps->busyHandler.base.jObj &&
-       (*env)->IsSameObject(env, ps->busyHandler.base.jObj, jBusy)){
+    JniHookState * const pHook = &ps->busyHandler;
+    if(pHook->jObj && (*env)->IsSameObject(env, pHook->jObj, jBusy)){
       /* Same object - this is a no-op. */
       return 0;
     }
-    rc = BusyHandlerJni_init(env, &ps->busyHandler, jBusy);
+    JniHookState_unref(env, pHook, 1);
+    pHook->jObj = REF_G(jBusy);
+    pHook->klazz = REF_G((*env)->GetObjectClass(env, jBusy));
+    pHook->midCallback = (*env)->GetMethodID(env, pHook->klazz, "xCallback", "(I)I");
+    IFTHREW {
+      JniHookState_unref(env, pHook, 0);
+      rc = SQLITE_ERROR;
+    }
     if(rc){
-      assert(!ps->busyHandler.base.jObj);
-      return (jint)rc;
+      return rc;
     }
-    assert(ps->busyHandler.base.jObj && ps->busyHandler.klazz);
-    assert( (*env)->IsSameObject(env, ps->busyHandler.base.jObj, jBusy) );
   }else{
-    BusyHandlerJni_clear(env, &ps->busyHandler);
+    JniHookState_unref(env, &ps->busyHandler, 1);
   }
   return jBusy
     ? sqlite3_busy_handler(ps->pDb, s3jni_busy_handler, ps)
@@ -1431,9 +1403,7 @@ JDECL(jint,1busy_1handler)(JENV_JSELF, jobject jDb, jobject jBusy){
 JDECL(jint,1busy_1timeout)(JENV_JSELF, jobject jDb, jint ms){
   PerDbStateJni * const ps = PerDbStateJni_for_db(env, jDb, 0, 0);
   if( ps ){
-    if( ps->busyHandler.base.jObj ){
-      BusyHandlerJni_clear(env, &ps->busyHandler);
-    }
+    JniHookState_unref(env, &ps->busyHandler, 1);
     return sqlite3_busy_timeout(ps->pDb, (int)ms);
   }
   return SQLITE_MISUSE;
diff --git a/manifest b/manifest
index cde7e0a83e..9324f7b684 100644
--- a/manifest
+++ b/manifest
@@ -1,5 +1,5 @@
-C Refactor\sthe\scollation-specific\sJNI\shook\stype\sto\suse\sthe\sgeneric\shook\stype.
-D 2023-07-31T09:45:49.092
+C Refactor\sthe\sbusy-handler-specific\sJNI\shook\stype\sto\suse\sthe\sgeneric\shook\stype.
+D 2023-07-31T10:08:36.744
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@@ -232,7 +232,7 @@ F ext/icu/icu.c c074519b46baa484bb5396c7e01e051034da8884bad1a1cb7f09bbe6be3f0282
 F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a32075a8
 F ext/jni/GNUmakefile 56a014dbff9516774d895ec1ae9df0ed442765b556f79a0fc0b5bc438217200d
 F ext/jni/README.md c0e6e80935e7761acead89b69c87765b23a6bcb2858c321c3d05681fd338292a
-F ext/jni/src/c/sqlite3-jni.c 31cf7df43d9c7e99431260e9ec0fbc622587e875ab22b3f4f6a262fd77bf16c8
+F ext/jni/src/c/sqlite3-jni.c 9dc18b6eec43132aa9a5001bc12ddd29c69513a4c4d04717b4131a16b6782906
 F ext/jni/src/c/sqlite3-jni.h 28def286ee305c1c89a43ac5918a6862d985d0534f7ccbbd74df4885d3918b73
 F ext/jni/src/org/sqlite/jni/BusyHandler.java 1b1d3e5c86cd796a0580c81b6af6550ad943baa25e47ada0dcca3aff3ebe978c
 F ext/jni/src/org/sqlite/jni/Collation.java 8dffbb00938007ad0967b2ab424d3c908413af1bbd3d212b9c9899910f1218d1
@@ -2071,8 +2071,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 f4aa2c82882cb6be1fd52977de19fd03c2e38abb857b520f951b32d610972ab6
-R 3fb71491075a1e3e33a1832011a5ad65
+P 02c1d3b6501fedf3d6e6d1ca60699df268522182c5ba3b49ae8f4691499ef0fc
+R 5b96073cb268292054640619b4e4117b
 U stephan
-Z 6de14785676dadf1719d78be6df968a1
+Z 2c77c8dd9c75c9f80fb562e45a42818f
 # Remove this line to create a well-formed Fossil manifest.
diff --git a/manifest.uuid b/manifest.uuid
index 690cb470a8..8ffd0dc238 100644
--- a/manifest.uuid
+++ b/manifest.uuid
@@ -1 +1 @@
-02c1d3b6501fedf3d6e6d1ca60699df268522182c5ba3b49ae8f4691499ef0fc
\ No newline at end of file
+d9efdc6dd20a34bfdaad5d4bf8e67cce7e35238299eb91e4459d59fda11978a6
\ No newline at end of file