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

Drop broken connections #12

Closed Profpatsch closed 2 years ago

Profpatsch commented 2 years ago

Fixes https://github.com/nikita-volkov/hasql-pool/issues/6

This PR has four parts, the first could be split out:

1) Add shell.nix, .gitignore, hie.yaml 2) Refactor use to be straightforward to read & understand (inline withResourceOnEither). 3) Based on @fghibellini’s commits in the issue, add a callback to the settings that can drop connections on QueryError. 4) Add a changelog and bump to 0.6.0

Section 2 has most of the commits, every one should be a plain refactor step with no functionality changes, leading to the inlining & documenting of use.

Section 3 is split in two, first the rebased commits from the issue, then finished by the commit connectionHealthCheck: rename to onQueryError and improve after the refactor.

There should be no functional changes apart from the code in defaultOnQueryError.

Profpatsch commented 2 years ago

Maybe important to note, I didn’t rebase the commit which changes the Pool datatype to throw on a failed connection, since it doesn’t make much sense in my books. The pool might need to generate new connections in arbitrary positions, so throwing out an error doesn’t make much sense and will make all client code brittle.

Profpatsch commented 2 years ago

Ping, will this ever be merged? I think it’s an important fix for any kind of production use.

nikita-volkov commented 2 years ago

Sorry for neglecting this.

This is an opinionated rewrite. Instead of arguing about the changes introduced here I want to encourage you to release your rewrite as your own package.

nikita-volkov commented 2 years ago

At your request I'll add that package to the ecosystem listing.

Thanks for your contribution!

Profpatsch commented 1 year ago

This is an opinionated rewrite.

That’s a euphmism for “oh no the code has comments now” but yeah. :)

I’m not married to this being merged, the library seems to have been rewritten a bunch in the meantime.