search

nikita-volkov / hasql-pool

A pool of connections for Hasql
http://hackage.haskell.org/package/hasql-pool
MIT License
17 stars 15 forks source link

Catches session exceptions #26

Closed periodic closed 2 years ago

periodic commented 2 years ago

Issue #25

I ran into an issue that if there was an exception thrown from a session, the pool does not return the connection or capacity to the pool and the pool can run out of connections!

This change has two parts:

  1. Catch session errors and surfaces them as SessionUsageErrors in the return value so it can be handled by existing logic on whether to reuse the connection or not.
  2. Catch other errors, returns the capacity to the pool, then re-throws the exception.

The first point is just to make sure that the errors we want to surface in the return are handled. Maybe they can never be thrown in normal usage, but with things like hasql-transaction-io you can get some very creative errors.

The second is the meat of the change. It catches other errors and makes sure capacity is returned to the pool. I opted not to return the connection in case the error was some sort of error indicating that the connection was no longer valid.

I tested this by running a session that was:

(use pool $ error "Query 1") `catch` \(e :: SomeException) -> use pool $ error "Query 2"

Without the change it would hang on the second query. With this change it threw the second error.

nikita-volkov commented 2 years ago

Thanks for the contribution! I have a few comments for discussion though.

I don't think, we should catch QueryError as exception, because Hasql never throws them. Actually this makes me reconsider the addition of the Exception instance for it, which I was skeptical about in the first place.

Regarding the catch of SomeException - I agree with that one, however I think it needs a correction. I don't see the reason to wrap the whole definition of the onConn function in catch. Instead I think wrapping just the session execution piece should be enough. E.g., using try @SomeException $ Session.run sess conn.

periodic commented 2 years ago

Great points. You are probably right. Now that I think about I'm not sure why I wrapped all of that. I'll make a few changes and send you an update shortly.

periodic commented 2 years ago

There you go, greatly simplified. Still does what I need it to do!