Final bikeshedding call: Fixing Control.Exception.bracket

Fri Nov 14 22:17:03 UTC 2014

Hi Edward,

I think both me and Eyal where taking the addition of
"interruptibleBracket" and variants for granted as an obvious part of
this proposal. I fully agree that there *should* be an
interruptibleBracket for people who want it. I just think it's the wrong
default and that 'bracket' should become the obvious default.

--
Merijn

On Fri, Nov 14, 2014, at 10:30, Edward Kmett wrote:
> It strikes me that this is winding up highly controversial.
>
> Assuming that, well, things might not go your way in terms of getting
> bracket changed, let's think a bit about what a more retrenched
> solution would look like.
>
> This way we have a continuum of fixes and can try to find the right
> point in the continuum.
>
> So let's put forth a couple more colors for the bikeshed:
>
> *Documentation Only*
>
> The issue is that users use bracket assuming things will be safer than
> they are. At the very least we need to clearly document the fact that
> this isn't the case!
>
> We could document the pattern of using uninterruptibleMask_ yourself
> in handlers for safety and include examples of where it is required.
>
> *Combinators*
>
> If we want to go further, we could introduce:
>
> uninterruptibleBracket :: IO a -> (a -> IO b) -> (a -> IO c) -> IO c
> uninterruptibleBracket acquire release = bracket acquire
> (uninterruptibleMask_ . release)
>
> uninterruptibleBracket_ :: IO a -> IO b -> IO c -> IO c
> uninterruptibleBracket_ acquire release = bracket_ acquire
> (uninterruptibleMask_ release)
>
> etc.
>
> and then upgrade the documentation for `bracket` and the like with a
> big fat warning about how certain common sense examples using bracket
> should really be using uninterruptibleBracket.
>
> *Merijn's Proposal*
>
> We could go further and switch the default behavior of bracket to that
> of uninterruptibleBracket above. Then we get Merijn's proposal.
>
> *Going Further*
>
> But if we do that we should probably consider adding an
> `interruptibleBracket` that matches the existing behavior with the
> same caveats we would want to put on `bracket`.
>
> The use cases that folks have that center around weird RPC handling
> scenarios in the release handler seem to fit into this niche.
>
> This redefines bracket, like Merijn would prefer, because it is a
> source of very very hard to track down resource bugs, and make the few
> who actually want to do complex code that relies on active
> asynchronous exception support in the handler switch combinators.
>
> It also has the benefit that the name interruptibleBracket is easier
> to explain than uninterruptibleBracket, which only made release
> uninterruptible.
>
> The kind of code that would be affected is the kind of code that
> would be very visibly affected, whereas the kind of code that is
> broken right now is scattered across the entire ecosystem and is just
> subtly wrong.
>
> *Personal Thoughts*
>
> I started writing this proposal with a continuum in mind, thinking I'd
> land somewhere in the middle.
>
> Normally, I'd be disinclined to change semantics on a function with
> such widespread use! I very strongly sympathize with Greg's
> position here.
>
> However, personally, I think the "Going Further" solution above is the
> right solution, which is effectively Merijn's proposal with the
> addition of interruptibleBracket and the like.
>
> However, in this case the only code that really can rely upon this
> behavior is code that happens to know it won't kill the thread from
> outside until it is in the handler, but that they want to do a thing
> that will throw them asynchronous exceptions _within_ the handler, and
> well, that is a marginal enough use case that I don't have much a
> problem marginalizing it further by forcing its practitioners to use a
> more exotic combinator. If they are absolutely allergic to the new
> semantics you can always swap all the existing uses of bracket to
> interruptibleBracket, and the very kind of user who would need these
> semantics is the kind of user who is equipped to carry out this sort
> of change.
>
> This effectively puts me at +1 with the caveat that I'd like to see
> interruptibleBracket added.
>
> -Edward
>
>
> On Tue, Nov 11, 2014 at 1:09 PM, Merijn Verstraaten
> <merijn at inconsistent.nl> wrote:
>> Ola!
>>
>>
In September Eyal Lotem raised the issue of bracket's cleanup handler
not being uninterruptible [1]. This is a final bikeshedding email before
I submit a patch.
>>
>>
The problem, summarised:
>>
Blocking cleanup actions can be interrupted, causing cleanup not to
happen and potentially leaking resources.
>>
>>
Main objection to making the cleanup handler uninterruptible:
>>
Could cause deadlock if the code relies on async exceptions to interrupt
a blocked thread.
>>
>>
I count only two objections in the previous thread, 1 on the grounds
that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't
believe this is a problem".
>>
>>
The rest seems either +1, or at least agrees that the status quo is
*worse* than the proposed solution.
>>
>>
My counter to these objections is:
>>
1) No one has yet shown me any code that relies on the cleanup handler
   being interruptible
>>
>>
2) There are plenty of examples of current code being broken, for
   example every single 'bracket' using file handles is broken due to
   handle operations using a potentially blocking MVar operation
   internally, potentially leaking file descriptors/handles.
>>
>>
3) Even GHC-HQ can't use bracket correctly (see Simon's emails)
>>
>>
Potential solution #1:
>>
Leave bracket as-is, add bracketUninterruptible with an uninterruptible
cleanup handler.
>>
>>
Potential solution #2:
>>
Change bracket to use uninterruptible cleanup handler, add
bracketInterruptible for interruptible cleanups.
>>
>>
Trade-offs:
>>
Solution 1 won't change the semantics of any existing code, however this
also means that any currently broken uses of bracket will remain broken,
possibly indefinitely.
>>
>>
Solution 2 will change the semantics of bracket, which means any
currently broken uses of bracket will be fixed, at the cost of
creating potential deadlocks in code that relies on the
interruptibility of cleanup.
>>
>>
I will argue that solution #2 is preferable, since I have yet to see any
code that uses the interruptibility of the cleanup handler. Whereas
there's many broken assumption assuming the cleanup handler is not
interruptible.
>>
>>
Secondly, it is easier to detect deadlocks caused by this problem than
it is to detect resource leaks which only happen in unlucky timings of
async exceptions. Especially since any deadlock caused by the change can
be fixed by replacing bracket with bracketInterruptible.
>>
>>
[4] -
    https://www.haskell.org/pipermail/libraries/2014-September/023675.html
>>
>>
Cheers,
>>
Merijn
>>
_______________________________________________
>>
Libraries mailing list
>> Libraries at haskell.org
>> http://www.haskell.org/mailman/listinfo/libraries
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/libraries/attachments/20141114/9da7d6f6/attachment.html>