Thread-safe default-connection

[kostafey]

I need to have possibility to use specific database connection to each request. How can I safe achive this for many simultaneous requests?

[ls4f]

Have you tried korma.db/with-db ? It accepts a structure, generated by korma.db/create-db. I'm currently using it in production with no complaints. Just in case it's not obvious - korma.db/default-connection is not thread-safe! Mixups don't happen too often and are hard to spot :)

[kostafey]

@ls4f, as I can see, with-db just temporary changes default-connection. But I can't recognize it as thread-safe. Am I mistaken?

[ls4f]

Well, thread-safe is not the proper term since it is not really related to threading. However, it does ensure that whatever you execute in it is done with the specified connection and you can keep your connections in a native clojure structure (which is immutable, therefore safe). You should know there is a catch (which you will notice soon enough on your own) - if you generate lazy sequences, they might be realized after you leave the block. It will either tell you there is no specified connection or possibly execute on some other default specified in the code. What I do is a simply doall for such things.

OK, took a better look at things. I think you are right and with-db is strangely implemented. What you could do instead and what I think is the right way to do it looks something like

(clojure.java.jdbc.deprecated/with-connection @(:pool db-spec) (do-fun-stuff))

Think I`ll make a pool request about that one:)

[kostafey]

I missunderstand the following point. If I run with-db and set default-connection in try block, then some other thread will run with-db with different connection, and my first thread will eval his body with this new connection, not the original.

[ls4f]

Yes, yes ... sorry about that one, I hadn't noticed how the macro is defined. You are free to call the jdbc macro directly (like Korma itself does at times). The dynamic var it rebinds should not be affected by different threads ... I think.

[kostafey]

Yes, clojure.java.jdbc aproach is thread-safe. It will be very nice to have such possibility in Korma API. BTW, is it possible to achive it without use deprecated methods? Probably we should ask clojure.java.jdbc maintainer to do not remove such function in future and don't consider with-connection deprecated.

[ls4f]

https://github.com/korma/Korma/commit/8dba8e52ae291570babd6cace0a533e60528c976

I think the change was done in order to work around the catch I mentioned (even if it does appear to break threaded environments). I Guess we`ll need to hear from the Korma guys on that one.

I don't think you'll be able to do it without the deprecated methods, since a major change is the removal of the dynamic var.

[kostafey]

8dba8e5

Exactly! It's the commit, wich makes multiple connection handling not thread-safe to my mind. But I am not sure that the reason is to avoid the laziness. Can somebody comment it? @ceterumnet

As I said, it is worth to ask clojure.java.jdbc author to don't consider with-connection deprecated if it will be used in Korma.

[ls4f]

Imagine (for the sake of argument) that you map a bunch of tables to their row counts. The returned sequence is lazy and might not ever be evaluated. Problem comes when you decide to go through it after you have left the jdbc/with-connection block, because at that time the dynamic var is no longer bound and the queries have nothing to work with. I assume that was a big reason to change the jdbc API and include a connection with every statement possible.

[kostafey]

I can't see the problem. jdbc/query returns the data structure you want. But it takes connection as an argument and don't breaks thread-safe for multitle connections. With old jdbc/with-connection you have to deal with you data in this block.

Why not use jdbc latest approach in Korma to take db-connection structure as an optional argument, for example, and use it as binding value and don't switch global atom _default value?

[ls4f]

The problem is the statement (say select) in such cases is not executed directly, but within the lazy sequence. It might not ever be executed :)

As for using the latest - that would take some time..

[kostafey]

It seems, you are right. It's impossible to satisfy my task requirements by Korma without big amount of work. Korma is useful to deal with a single database and not with multiple databases at the same time. Ok. But keep my request in mind :).

[immoh]

Thanks for bringing this up. You're right, with-db is no longer thread-safe and I consider this as bug that needs to be fixed.

[ceterumnet]

@immoh - in a previous pull request, I wasn't modifying the default connection directly, but rather I was using a separate binding for each with-db - is this the direction we might want to go? I believe that should make it thread-safe again.

[ceterumnet]

@immoh and I would be happy to create a new pull request that makes with-db thread-safe if you are interested.

[immoh]

@ceterumnet Sure, pull requests are welcome & very much appreciated.

I'd like to see this implemented without new dynamic var, not sure if it's possible though. I am also afraid that if we introduce new dynamic var for it, it will cause other bugs with default connection. Maybe we should make default connection var dynamic and make a thread-local binding in with-db?

[ls4f]

Isn't it best just to add another macro with the old behavior? That way everyone can make the choice for themselves (and if needed - take care of any laziness issues).

[ceterumnet]

I don't think we need the old behavior per se - I've prepared a new pull request -> https://github.com/korma/Korma/pull/236

This pull request introduces a new dynamic var.

I think this should make all parties happy.

[ls4f]

Appears ok ... but am I the only one who thinks it's a bad idea to introduce a dynamic var, considering jdbc got rid of it?

Also even if we decide to go that way, I don't think you need the dynamic var within transaction and do-query. They check jdbc/find-connection, so with-db can simply call jdbc/with-connection. The new dynamic var is needed only in assoc-db-to-entity (I think). Now that I'm saying it - can't we just use the jdbc/*db* over there and skip the additional var?

[ceterumnet]

I'm not sure how we can do it without the dynamic var given the way that certain associations are evaluated. Even with jdbc/find-connection, the problem is that the lazy evaluation will occur outside of the jdbc/with-connection.

I've updated the pull request to eliminate the dynamic var in do-query and transaction.

[ls4f]

Yes, that is exactly what I was thinking. Are you sure you can't use the jdbc/*db* var in assoc-db-to-entity (it will be bound basically at the same times the new var is) ?

[ceterumnet]

I hadn't thought about the jdbc/db possibility. I just tried it out, and it appears that the db is a private var ...

I suppose all of this could be rethought for a future release. I would really like to see the db be passed into every function that communicates with the database as a required argument and eliminate the concept of a default connection altogether. However, I'm not sure how folks feel about that.

I suppose I would have to read all of the previous discussions etc...and understand the overall motivations behind the korma architecture in order to make more intelligent feedback :)

[ls4f]

First of all - I am thinking of migrating things to the new JDBC API. Can't promise when/if I'll be ready with something to test, though.

As for the var being private:

(defn find-connection
  "Returns the current database connection (or nil if there is none)"
  ^java.sql.Connection []
  (:connection *db*))

Which should mean you can call the function at that place and use the jdbc/*db* var that way. That's what I meant earlier about not introducing a second dynamic var here.

[ceterumnet]

So I think the problem is that their is a type mismatch using that function with the way the rest of the code is written.

find-connection returns the connection - whereas with-connection expects a db-spec ... so I think those are really 2 different things. The db-spec includes the references to the connection pool etc...whereas the connection is sourced from the db-spec...

Unless I am not understanding this correctly..

[ls4f]

Darn ... you have a point there. Not really sure the full db spec would be needed at that exact spot but I guess a new var is a better way to go at it in the short term than redoing the interface.

[immoh]

Fixed in 0.3.2.

[kostafey]

Thank you!