Repository: xerial/sqlite-jdbc
Pull request #843 (2023-02-28 13:00:21)

fix SQLiteConfig keep cached busyTimeout more consistent with busy_timeout pragma
I noticed while setting up the DataSource that the SQLiteConfig busyTimeout was getting out of synch with the busy_timeout pragma when doing something like the following:

```
final SQLiteDataSource ds = new SQLiteDataSource();
ds.getConfig().setBusyTimeout(2000);
```

This PR fixes this inconsistency. Note that it is still possible to set the pragma value directly and get out of synch, for example:
```
final SQLiteDataSource ds = new SQLiteDataSource();
ds.getConfig().setBusyTimeout(2000);
ds.getConfig().setPragma(Pragma.BUSY_TIMEOUT, "5000");
```

But, at least with this patch, you'd have to go out of your way to get out of synch, and for the normal usage, these values would now be kept in synch.

EDIT: added a second commit with a new test.

gotson: why not just:
- remove the field `SQLiteConfig.busyTimeout`
- change the implementation of `SQLiteConfig.getBusyTimeout` to always retrieve the value from the pragmaTable

Then it should work even in your second example:

```java
final SQLiteDataSource ds = new SQLiteDataSource();
ds.getConfig().setBusyTimeout(2000);
ds.getConfig().setPragma(Pragma.BUSY_TIMEOUT, "5000");
```
mruddy: `SQLiteConfig.busyTimeout` is just a cache of the pragma value because the pragma is stored as a `String` and `SQLiteConfig.getBusyTimeout` winds up getting called for each executed statement. So, to answer your question, it's there to prevent parsing a String to an int for every statement execution. Just a performance thing.
gotson: > `SQLiteConfig.busyTimeout` is just a cache of the pragma value because the pragma is stored as a `String` and `SQLiteConfig.getBusyTimeout` winds up getting called for each executed statement. So, to answer your question, it's there to prevent parsing a String to an int for every statement execution. Just a performance thing.

Given it's never gonna be called often, I would prefer the parsing impact instead of the potential state inconsistency. 
mruddy: just added two commits to update the spotless-maven-plugin version config and then apply the formatting updates to the 2 files changed by this pull request. the formatting changes made by the plugin wrapped a few lines and reformatted some comments.
gotson: > just added two commits to update the spotless-maven-plugin version config and then apply the formatting updates to the 2 files changed by this pull request. the formatting changes made by the plugin wrapped a few lines and reformatted some comments.

Hi, please don't update the deps in this PR, this has nothing to do with this change 
mruddy: i only updated the plugin version, not the dependencies. i updated it because the CI build failed the "spotless" check. then when i went to apply the spotless plugin, it complained at me to update it. that's why i added the last two commits.
gotson: > i only updated the plugin version, not the dependencies. i updated it because the CI build failed the "spotless" check. then when i went to apply the spotless plugin, it complained at me to update it. that's why i added the last two commits.

you just need to run `mvn spotless:apply`, no need to update any deps
mruddy: when i ran `mvn spotless:apply` i kept getting
```
java.lang.Exception: google-java-format 1.11.0 is currently being used, but outdated.
google-java-format 1.15.0 is the recommended version, which may have fixed this problem.
google-java-format 1.15.0 requires JVM 11+.
```
i can take out commit f304b1bdbd42c09ae1ece832cb35a46492b8afee as that is the only pom change that i made.
gotson: > i can take out commit [f304b1b](https://github.com/xerial/sqlite-jdbc/commit/f304b1bdbd42c09ae1ece832cb35a46492b8afee) as that is the only pom change that i made.

yes thank you
mruddy: done. i went back and dropped those last 2 commits and then found the runtime that the ci build is using so that i could run the spotless checks and get matching results when running `mvn spotless:apply`.

for reference, i got the jdk from:
https://adoptium.net/temurin/archive/?version=11
https://github.com/adoptium/temurin11-binaries/releases/tag/jdk-11.0.18%2B10

Patch:
diff --git a/src/main/java/org/sqlite/SQLiteConfig.java b/src/main/java/org/sqlite/SQLiteConfig.java
index 3fc2301d..ee84f470 100755
--- a/src/main/java/org/sqlite/SQLiteConfig.java
+++ b/src/main/java/org/sqlite/SQLiteConfig.java
@@ -54,7 +54,7 @@ public class SQLiteConfig {
     private final Properties pragmaTable;
     private int openModeFlag = 0x00;
 
-    private final int busyTimeout;
+    private int busyTimeout;
     private boolean explicitReadOnly;
 
     private final SQLiteConnectionConfig defaultConnectionConfig;
@@ -87,8 +87,8 @@ public class SQLiteConfig {
         // Enable URI filenames
         setOpenMode(SQLiteOpenMode.OPEN_URI);
 
-        this.busyTimeout =
-                Integer.parseInt(pragmaTable.getProperty(Pragma.BUSY_TIMEOUT.pragmaName, "3000"));
+        setBusyTimeout(
+                Integer.parseInt(pragmaTable.getProperty(Pragma.BUSY_TIMEOUT.pragmaName, "3000")));
         this.defaultConnectionConfig = SQLiteConnectionConfig.fromPragmaTable(pragmaTable);
         this.explicitReadOnly =
                 Boolean.parseBoolean(
@@ -1162,6 +1162,7 @@ public class SQLiteConfig {
     /** @param milliseconds Connect to DB timeout in milliseconds */
     public void setBusyTimeout(int milliseconds) {
         setPragma(Pragma.BUSY_TIMEOUT, Integer.toString(milliseconds));
+        busyTimeout = milliseconds;
     }
 
     public int getBusyTimeout() {


Test patch:
diff --git a/src/test/java/org/sqlite/SQLiteConfigTest.java b/src/test/java/org/sqlite/SQLiteConfigTest.java
index b0457d43..8980d20a 100644
--- a/src/test/java/org/sqlite/SQLiteConfigTest.java
+++ b/src/test/java/org/sqlite/SQLiteConfigTest.java
@@ -25,4 +25,29 @@ public class SQLiteConfigTest {
         assertThat(properties.getProperty(SQLiteConfig.Pragma.DATE_CLASS.getPragmaName()))
                 .isEqualTo(SQLiteConfig.DateClass.REAL.name());
     }
+
+    @Test
+    public void setBusyTimeout() {
+        SQLiteConfig config = new SQLiteConfig();
+
+        // verify the default is set in the pragma table and the cached value
+        assertThat(config.toProperties().getProperty(SQLiteConfig.Pragma.BUSY_TIMEOUT.pragmaName))
+                .isEqualTo("3000");
+        assertThat(config.getBusyTimeout()).isEqualTo(3000);
+
+        // verify that the default is updated in both places
+        config.setBusyTimeout(1234);
+        assertThat(config.toProperties().getProperty(SQLiteConfig.Pragma.BUSY_TIMEOUT.pragmaName))
+                .isEqualTo("1234");
+        assertThat(config.getBusyTimeout()).isEqualTo(1234);
+
+        Properties properties = new Properties();
+        properties.setProperty(SQLiteConfig.Pragma.BUSY_TIMEOUT.pragmaName, "100");
+        config = new SQLiteConfig(properties);
+
+        // verify that we can set an initial value other than the default
+        assertThat(config.toProperties().getProperty(SQLiteConfig.Pragma.BUSY_TIMEOUT.pragmaName))
+                .isEqualTo("100");
+        assertThat(config.getBusyTimeout()).isEqualTo(100);
+    }
 }