Allow DB::Pool to be used a generic connection pool #131
Conversation
| res | ||
| end | ||
|
|
||
| def checkout(&block : T ->) |
There was a problem hiding this comment.
I'm not sure. But the new Pool#checkout(&) might not play nice with DB connections. It's not documented, but the auto_release property handles the logic of how the resource is released after the operation is completed. This requires some knowledge of the resource though. Maybe we need to formalise that a bit better.
There was a problem hiding this comment.
Interesting. I haven't looked into how that works but it sounds like it might fall under DB-specific extensions that could be made if this class were extracted to its own shard, which DB::Pool could then inherit from.
examples/http_client.cr
Outdated
| @@ -0,0 +1,8 @@ | |||
| require "http" | |||
There was a problem hiding this comment.
I think we can create specs to ensure the pool is generic.
There was a problem hiding this comment.
I had a feeling you might say that 🙂 I didn't know how to do it, though, so I figured I'd at least offer up what I had.
To clarify, I've always struggled with testing I/O. I don't know that we'd have to actually test I/O here, but that hadn't crossed my mind when I was writing this.
examples/http_client.cr
Outdated
| require "http" | ||
| require "../src/db/pool" | ||
|
|
||
| pool = DB::Pool.new { HTTP::Client.new(URI.parse("https://google.com")) } |
There was a problem hiding this comment.
If we want DB::Pool to be used directly maybe the API for setting the pool options should be refactored a bit.
Otherwise it will be too fragile when new options are coming.
src/db/error.cr
Outdated
| @@ -14,10 +14,10 @@ module DB | |||
| # Raised when an established connection is lost | |||
| # probably due to socket/network issues. | |||
| # It is used by the connection pool retry logic. | |||
| class ConnectionLost < Error | |||
| getter connection : Connection | |||
| class ConnectionLost(T) < Error | |||
There was a problem hiding this comment.
Note to future self: check if this is causing any breaking change on db drivers
There was a problem hiding this comment.
I've checked this against pg and it worked fine. I didn't check the other drivers, though.
Co-authored-by: Brian J. Cardiff <bcardiff@gmail.com>
|
@jgaskins I checked the status of this
The pool still requires that the resource is a Disposable (or at least has a close method) something that the HTTP::Client has. So far if this status is useful to you, we can add those commits here and merge this PR. |
|
@bcardiff That sounds great. The specs look good! Nice catch on the ICE, too — but what does that stand for? I was aware of the requirement that it respond to Are you able to push to that branch on my fork since I opened this PR or do you need me to pull them in? |
|
ICE = Internal Compiler Error. Yup, I didn't want to push before asking. |
|
Awesome, thank you for your help with this. I really wasn't sure how to automate the tests for that and, looking at the ones you wrote here, I feel like it would've taken me a lot of time and experimentation to arrive at anything resembling a useful spec, so I really appreciate your effort there. |
…ption Otherwise the rescue ConnectionLost(T) in the Pool might will not catch when T is a driver specific connection
|
Welcome. The support files are 99% the ones in the std-lib from the http server specs and fibers specs. It's hard to express exactly the expected interactions when dealing with fibers and ensuring the specs will not hang forever is something that requires extra attention. I needed to add one more fix and that is effectively a breaking change, but required to allow the right type of exception to be raised and caught in the pool. If nobody complains about that, we can merge this in a couple of days :-) |
|
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/pooling-http-connections-automatically/7030/1 |
This allows an application to use
DB::Poolfor all of its connection pools. I've included a couple examples:HTTP::Client— the idea is that you'd wrap aPoolinstance to make your own HTTP API clientIOobject which I was using to check that concurrency was still workingI'd be cool with removing the examples. They're just what I was using to make sure this was truly generic.
Fixes #122