seancorfield / next-jdbc

A modern low-level Clojure wrapper for JDBC-based access to databases.
https://cljdoc.org/d/com.github.seancorfield/next.jdbc/
Eclipse Public License 1.0
768 stars 90 forks source link

->pool for HikariCP should call the constructor checking the connection #179

Closed holyjak closed 3 years ago

holyjak commented 3 years ago

Currently connection/->pool will call the no-arg HikariDataSource constructor, which means it will bypass the initial connection checking, which users might have configured (on by default). It means that problems with reaching the DB will not be discovered until a connection is actually requested from the pool.

It is possible to make sure that the correct, checking constructor is called via this hack (where we first instantiate HikariConfig, then create the DS and pass it it as an argument):

(HikariDataSource.
    (jdbc.connection/->pool
      com.zaxxer.hikari.HikariConfig
      {:dbname org-label, ...}))

Ideally (for me, as the user) next-jdbc would do this for me, though I imagine we do not want its code to depend on HikariCP code, which makes it difficult. So at least the conn. pooling docs could be updated with this info... (should we recommend the reader to (defmethod j/to-java [com.zaxxer.hikari.HikariDataSource clojure.lang.APersistentMap] ...)?

I guess this would work:

(require '[clojure.java.data :as j])

(defmethod j/to-java [HikariDataSource clojure.lang.APersistentMap] [_ props]
  (HikariDataSource.
    (j/to-java HikariConfig props)))

(connection/->pool HikariDataSource db-spec)
seancorfield commented 3 years ago

Given that ->pool has to be generic -- for at least c3p0 and HikariCP -- do you have a suggestion on how to resolve this within next.jdbc or is this just for the docs? Also, does this constructor invocation hack leave you with two active pools or just one? (i.e., would the original one need to be closed/shutdown)

holyjak commented 3 years ago

No, I have no good idea other than some reflection dark magic. My solution doesn't create two pools, the no arg constructor (seems to) souřadnice init the pool.

seancorfield commented 3 years ago

I just looked at the source. The no-arg constructor doesn't start a pool and it isn't started until getConnection() is called the first time. The copy-constructor builds a new datasource from the config underlying what it is passed -- not from the datasource portion -- and then eagerly starts a new pool.

So you'll get just one pool as long as you don't call getConnection() on the datasource constructed via ->pool.

Because of the lazy construction of the pool on the first call to getConnection(), it does still call validate() -- it just does it in a different place: https://github.com/brettwooldridge/HikariCP/blob/HikariCP-3.4.5/src/main/java/com/zaxxer/hikari/HikariDataSource.java#L109

Given that, I think your constructor hack is not actually required, and this issue can be closed?

holyjak commented 3 years ago

You are right. So what I do now is

(let [ds
        (jdbc.connection/->pool HikariDataSource db-spec)]
    ;; Force initialization of the pool and checking of the DB
    ;; (see https://github.com/seancorfield/next-jdbc/issues/179):
    (with-open [_ (.getConnection ds)])
    ds)

and that works fine. I will send a PR to add that to the docs.