Hikari blindly overrides Connection's readOnly and autoCommit

[xathien]

Environment

HikariCP version: 2.3.8 and master
JDK version     : 1.8.0_151
Database        : PostgreSQL|MySQL|any
Driver version  : 9.4.1208

---

Have you searched the CLOSED issues already? How about checking stackoverflow?
Yes.

Bug

If readOnly or autoCommit are specified via the JDBC URL, HikariCP will blindly override them when calling setupConnection (both in PoolUtilities for 2.3.x and PoolBase for 2.4.x and the dev branch).

Proposed Resolution

Hikari should only call connection.setReadOnly and connection.setAutoCommit if the setting was directly specified.

[brettwooldridge]

@xathien What I propose is this:

• If the result of Connection.isReadOnly() is equal to HikariConfig.getReadOnly(), then Connection.setReadOnly() will be skipped.
• If the result of Connection.getAutoCommit() is equal to HikariConfig.getAutoCommit(), then Connection.setAutoCommit() will be skipped.

This change will be made on v2.4.x. The v2.3.x branch is in maintenance and only critical bugs will be fixed. I cannot stress how much HikariCP has changed between the v2.3 and v2.4, in terms of stability, performance, and number of issues resolved. As you are on Java 8, I strongly recommend upgrading to the v2.4.x release train.

[xathien]

Thanks for the response, Brett! I'm not certain your proposed solution will quite fix the issue, though. If Connection.isReadOnly() == true due to &readOnly=true on the JDBC URL, then Hikari will still call con.setReadOnly(false) because we didn't explicitly say HikariConfig.setReadOnly(true).

If HikariConfig.readOnly was a tri-state Boolean instead, this problem could be alleviated by only calling Connection.setReadOnly() if HikariConfig.readOnly != null, meaning that the value was explicitly specified.

[brettwooldridge]

@xathien I acknowledge that is an edge case. However, it is also unsupported.

There are lots of settings that are valid to configure via the JDBC URL, but settings that directly conflict with pool controlled parameters are not among them.

This conflict appears in other contexts as well. For example, connectionInitSql can be set to SET TRANSACTION LEVEL READ ONLY. This is simply unsustainable in terms of support.

[brettwooldridge]

@xathien On a final note, changing the booleans to tri-state would be a potentially breaking change to several hundred thousand existing users.

It is both unknown and unknowable how many users do not set readOnly or autoCommit because the defaults were what they wanted, yet the driver they are using has different defaults. In this case, HikariCP was configuring the driver as they wished and expected based on the documented behavior. Suddenly not setting these values, in combination with tri-state booleans, would result in driver-default behavior in place of previously documented HikariCP behavior.

Finally, I am curious as to what your concern over the impact of setReadOnly() and setAutoCommit() is exactly, given that they occur during initial connection setup and then never again during the life of the connection in the pool?

[xathien]

Hey, thanks for replying. It seems doubtful I'll sway you from here, but I'll still do my best to explain my concerns.

The main impetus for this bug was the fact that I set readOnly in accordance with my driver's documentation (not realizing Hikari had special config for it) and lost several hours trying to figure out why my tests were failing. 🙂

My company's internal tooling abstracts Hikari pool creation away from me, so now we need to update that in case our chosen connection pooling library ignores the driver's settings.

In any case, the fact that this bug exists (albeit closed) may help someone else diagnose their issue after Googling for it.

[brettwooldridge]

@xathien If it makes a difference, I can investigate logging a warning if HikariCP detects URL parameters in conflict with pool controlled properties.

Given roughly half a dozen or so most used databases (MySQL/MariaDB, PostgreSQL, MS SQL Server, Oracle, ...) and likely only two or three pool parameters at issue, it seems well within possibility to implement such a warning. What do you think?

[xathien]

Having a warning would definitely have saved me a few hours, and may be a preferable compromise to a breaking change. 🙂

[chrisribble]

It would be even more useful if there was a setting one could flip that would suppress each of these behaviors; i.e. setInitializeReadOnly(boolean), etc. That would make it much easier to get the same behavior as other connection pools which do not override these values on check out.

I have the same use case as @xathien where internally we abstract connection pool configuration at a higher level (avoids need for each app to contain the same configuration logic and allows us to swap connection pools with a build classpath change).

I'm adding support for HikariCP (we currently use c3p0) and this just bit me as well for HSQL databases that are read-only by default. I can add yet-another-configuration-parameter that just doesn't do anything for c3p0 (since it doesn't appear to have an analogous configuration parameter), but would prefer to just tell HikariCP not to force changing the read-only state.

Btw, c3p0 provides a "connectionCustomizerClassName" configuration parameter to enable a user to customize the way that properties get set on newly-created connections. I wonder if a similar concept would be useful here, perhaps as a way to override the default behavior applied when setting up the connection. That might allow for a bit more flexibility than a bunch of booleans that try to anticipate whether to set every possible initialization parameter.

EDIT: Upon further inspection of the c3p0 implementation, I'd suggest that copying what they do precisely isn't quite flexible enough; specifying the name of a class that implements the interface they want isn't actually that useful if you want to dynamically configure these properties based on some provided configuration information. It would be better if a user could provide an instance of some concrete implementation of the interface.

[brettwooldridge]

@chrisribble The latest version of HikariCP, v3.0.0 at the time of this writing, does not call setReadOnly() or setAutoCommit() if the value returned by the driver matches the pool settings. Change here.