Fix issue #90: Timeout handling is incompatible with Hibernate

[kkofler]

The handling of the queryTimeout in JDBC3Statement was not compliant to the JDBC specification in several ways, breaking Hibernate:

• The queryTimeout was being applied to the whole connection, as soon as setQueryTimeout was called, and never set back. So setting the timeout on a single statement would affect the whole connection, even if the statement was about to be discarded (and thus its queryTimeout should have been irrelevant). The fix now only applies the timeout to the connection if and when the statement is actually executed, and resets it to the connection's original setting after the statement is done (using a try-finally block).
• A queryTimeout of 0 was translated to a busyTimeout of 0, which is not correct, because JDBC defines a queryTimeout of 0 to mean "there is no limit" (i.e., wait forever), whereas for SQLite, a busy_timeout of 0 means "do not wait at all", i.e., the exact opposite. We now only forward non-zero query timeouts to the connection. If queryTimeout=0 (the default), we keep the connection's default busyTimeout.
• The getQueryTimeout method was returning a value in milliseconds rather than seconds. This is now fixed simply by returning the internal queryTimeout variable, which is in seconds. (That change was necessary anyway because we no longer always set the connection's timeout.)

The first two issues together were breaking Hibernate because, for some reason, whenever Hibernate closes a Statement, the last thing it does before calling close() on the Statement is that it calls setQueryTimeout(0) on it. This was being misinterpreted as a setBusyTimeout(0) on the entire Connection. As a result, later requests on the same connection did not handle concurrency at all. (Hibernate treats the "database is locked" exception as basically a fatal error.) While Hibernate's behavior is strange, I think it is compliant to the JDBC specification, and SQLite-JDBC was doing the wrong thing. Fixing either of the two issues would have been enough to make Hibernate work, but this patch addresses both, so that we do what the JDBC documentation says.

Fixes #90.

[gitblit]

After reviewing your changes I've noticed that you did not apply your proposal to executeBatch. Was that an oversight or insight?

I'm wondering if it would be better to move queryTimeout to CoreStatement and then apply your query timeout logic in DB.prepare and DB.finalize, rather than replicating this logic 3 times.

[kkofler]

Was that an oversight or insight?

The latter: executeBatch calls executeUpdate to do the actual work, so the timeout should come from there. Or am I missing something?

I'm wondering if it would be better to [...]

Quite likely. I'm not really familiar with the code base.

[gitblit]

executeBatch calls db.executeUpdate not JDBC3Statement.executeUpdate. I don't think the timeout is considered for the batch call. I'm not sure if it should or should not be considered - hence my inquiry on oversight vs insight.

[MartinHaeusler]

I bumped into this problem too. Is there any particular reason why the pull request was never merged?

[oblodgett]

Any movement on this? I am running into this problem also.

[hasB4K]

We encountered this problem as well. A fix on this could be useful ! :)

[johannespostler]

For those still running into the Hibernate / Spring problems: checkout our fork (with a PR to this repo):

https://github.com/Txture/sqlite-jdbc

[kkofler]

@johannespostler: I do not see a pull request from you, and neither my nor your (apparently actually @MartinHaeusler's) version of the fix is merged upstream (after 6 years!). Can you please submit a pull request?

[johannespostler]

@kkofler I'm on the same team with @MartinHaeusler . His PR is the one to be merged: https://github.com/xerial/sqlite-jdbc/pull/432 . We have been rebasing our fork every now an then and haven't had the issue in years. Please do let me know if I can help in any way.

[kkofler]

Ah, so the timeout handling is part of a larger pull request, that's why I had not found it.

I have found the timeout fix alone to be pretty effective at preventing SQLITE_BUSY around here, but it might not be sufficient for all usage patterns.

[MartinHaeusler]

@kkofler The timeout handling makes SQLite-JDBC compatible with Hibernate (i.e. it basically works). However, it's easy to produce the dreaded SQLITE_BUSY errors, especially in high load scenarios. This exception is very hard to deal with in Hibernate; a simple retry is often not possible due to side-effects in transactional methods. What our fork does (in addition to timeout handling according to the JDBC/JPA spec) is:

• We look at the readonly flag which is set by Spring on all @Transactional methods. This flag is forwarded to the JDBC driver.
• If we detect that readonly is false, we immediately fire a BEGIN EXCLUSIVE command on the fresh JDBC connection. This forces an exclusive access. This is different from the regular SQLite behaviour; according to the SQLite docs, all transactions start out as read-only and are then upgraded to read-write when the first INSERT or UPDATE command occurs via SQL. This "upgrading" from read-only to read-write can fail, producing the SQLITE_BUSY error. By starting the transaction as exclusive right from the get go, we avoid this issue for the most part.
• If readonly is true, we do a BEGIN TRANSACTION instead.

This prevents the vast majority of cases where SQLite throws SQLITE_BUSY while retaining a reasonable amount of concurrency. It is, of course, not possible for two writers to access the database at the same time with this modus operandi. It's the only setup we've found to be feasible (after a lot of experimentation). There's still one caveat: certain concurrent interleavings of transactions (with at least one writer involved) may cause deadlocks within SQLite itself. This is a very rare scenario though and depends on the actual tables which have been accessed by the transactions.

[gotson]

For those still running into the Hibernate / Spring problems: checkout our fork (with a PR to this repo):

https://github.com/Txture/sqlite-jdbc

I am now helping to maintaining the repo, and will be reviewing and merging PRs. Given you seem to have made some changes on your end, would you be willing to help here:

• if there are open PRs here that were merged on your fork, adding a comment on those PRs to provide some feedback
• if you have other changes that are not here, would you be willing to make PRs ?

---

@kkofler would you be able to resolve the conflicts on this PR ?

[MartinHaeusler]

@gotson we have no more PRs on our branch and also no other open changes; it's all in our branch of the repo.

I could write an entire book about the interplay between JDBC, JPA and SQLite at this point, but I'll try to keep it short.

• Timeout Handling: The JDBC specification says that a connection timeout of 0 means "no timeout". The xerial-jdbc driver unfortunately interprets this as "time out immediately". We fixed this in our branch. This one is pretty clear, it's a bugfix and I don't see any use case for "time out immediately".
• Respecting Read-Only: JPA in general and Spring in particular distinguish pretty clearly between read-only and read-write transactions. This information is passed down to the JDBC driver as a boolean setting. For the specific case of SQLite, this differs from its usual mode of operation. In SQLite, all transactions start out equally as read-only, but when an update statement is received, they are "upgraded" to a higher isolation level. Our patch prevents this upgrading and throws an exception instead if the read-only setting has been received initially.
• Preventing SQLITE_BUSY: It is not an exaggeration to say that the error "SQLITE_BUSY" has been the bane of our existence with SQLite in the past three years. As mentioned previously, upgrading a transaction to a read-write transaction very frequently causes SQLITE_BUSY. We prevent this in our patch by calling BEGIN EXCLUSIVE on every read-write transaction. The advantage is that no upgrading ever needs to happen, reducing the likelihood of SQLITE_BUSY. However, users may experience random "lag" in the application because SQLite is waiting for some locks. Furthermore, it's not a universal solution because there are still cases where SQLite can deadlock (especially under heavy load from multiple users). The original intention of SQLITE_BUSY was to tell the application "hey, sorry, this transaction cannot be handled at this point, please replay it and try again". Well... to the best of my knowledge there is no mechanism for retries in JPA, at least none that doesn't require re-executing the business logic (which may have side effects, like sending e-mails, billings, etc.). So for JPA, SQLITE_BUSY is a "game over" message.

Feel free to look through our changes (the differences to the master branch are actually not that big). I admit that some of it is quite hacky, but we did what we had to do in order to make it work. Especially for the last bullet point I'm not sure how universally applicable it is.

Long story short: don't use SQLite for server workloads. It's nobodys fault, it's just not made for that.

[gotson]

Hi @MartinHaeusler,

If i understand correctly:

• timeout handling: this PR
• respecting read-only: #432
• Preventing SQLITE_BUSY: is there a PR for this, or is that a general statement ?

Feel free to look through our changes (the differences to the master branch are actually not that big). I admit that some of it is quite hacky, but we did what we had to do in order to make it work. Especially for the last bullet point I'm not sure how universally applicable it is.

I will not do that. If you have more PRs that this one and #432, please feel free to send them along. As i said, i am now helping to maintain this repo, and will get through all the open issues (tagging them) and PRs (merging them if they are worthy).

Can you fix the conflicts of this PR, so i can review it and hopefully merge it?

[gotson]

Feel free to look through our changes (the differences to the master branch are actually not that big). I admit that some of it is quite hacky, but we did what we had to do in order to make it work. Especially for the last bullet point I'm not sure how universally applicable it is.

I understand now, had a glance at your branch, and you only have a master branch with 13 commits, all part of #432

Will have a look at that PR soon.

[gotson]

Will also close #47 i believe