Fix resource pool regression in persistent-mysql-haskell, persistent.

[swamp-agr]

This PR fixes regression introduced in resource-pool-0.5.0.0. It sets bracket on connection errors and makes closing connection uninterruptible. Second commit fixes loggedClose from persistent by making it uninterruptible since it could be interrupted, resource-pool is no longer guaranteeing that.

[swamp-agr]

Note: this is a PR into "persistent-mysql-haskell-9.2" branch of the current fork.

[cdfa]

Will try to have a look at the code tomorrow, but since this resource-pool update also caused issues in hedis and I would expect a library like resource-pool to not make it so easy to leak things, I'm wondering why the behaviour was changed this way and why we aren't fixing it more "centrally". Do you know?

[swamp-agr]

Unfortunately, that is by design and that is why resource-pool bumped the major version (from 0.4 to 0.5).
From changelog for 0.5.0.0:

Stop running resource freeing functions within uninterruptibleMask.

destroyResource no longer ignores exceptions thrown from resource releasing functions.

• Masking:
  • In 0.4.x it was uninterruptible.
  • Since 0.5.x it has become interruptible.
• exception handling:
  • In 0.4.x it silently ignored all exceptions.
  • Since 0.5.x it has started loudly allowing throwing exceptions.

The reason why they added this was inability to destroy the resource by throwing an asynchronous exception from another thread. For instance timeout set on killing signal would not work in some cases, leading to never-ending graceful shutdown of the resource.

And of course, silently swallowing exceptions will lead to situations where such bugs would be very hard to catch. The same reason why we made a change Redis-schema on decoding errors.

[cdfa]

If resource-pool doesn't swallow exceptions anymore now, wouldn't the exceptions be logged twice with this now?

[cdfa]

After looking at the issue that was fixed by the resource pool change and the PR in which the change was implemented, I remain unconvinced that the change is an improvement.
I don't think interrupting free actions is something you usually want to do, so I don't understand why the resource-pool would make that possible at the cost of demanding that users keep asynchronous exceptions in mind all the time, especially when the workaround for when you do want to use them is quite simple.

If anything, resource-pool could have changed the API such that the free action receives the release function from uninterruptibleMask as an argument so the user is reminded that they need to unmask if the thread can be interrupted safely.

If you agree, I think we should take up discussion with the resource-pool author on this and fork if needed.