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

Force the timeout option to be specified #22

Closed nikita-volkov closed 1 year ago

nikita-volkov commented 2 years ago

I cannot imagine a case where it wouldn't make sense to have the timeout option specified. From that perspective there's no point in wrapping the value in Maybe.

@robx What do you think?

robx commented 2 years ago

I was thinking that there might well be users who don't want any timeout. Then it's a pain to configure an arbitrarily high timeout to effectively disable it. It also makes the library a bit less convenient to use if you have to make that decision whenever you create a pool. At least using this in PostgREST I was happy to not have to make a decision for a default value and instead allow the user to specify a time and default to "off"...

(There's also the option of dropping the Maybe and deactivating timeouts when the value is 0, but that feels a bit wrong. Or maybe of setting a sensible default, but I can't really think of any that makes sense.)

Do you expect most users to want to set a timeout?

nikita-volkov commented 2 years ago

Then it's a pain to configure an arbitrarily high timeout to effectively disable it.

Sure it's an extra place the user will have to think of. However it is something that he better think of either way IMO. When a user specifies say a year-long timeout, he is forced to think whether he really wants his app to stall waiting for a connection for a year. Likely not. With that said then having no timeout at all starts to look more questionable: is there any scenario at all where having the app stalled forever waiting for a connection is what the user wants?

Either way, I'm not super opinionated on the matter. I'm okay with keeping things as they are.

periodic commented 1 year ago

Just to chime in. We are using this library in production. We were recently debugging some concurrency issues and found ourselves saying, "we really should have put a timeout on this to rule out it getting stuck creating a connection".

Setting it to Nothing let us defer deciding on a timeout, which may not have been the right call.

nikita-volkov commented 1 year ago

This is now enforced in the master branch.