Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

Thank you for summarizing it, because the thread really run out of
control (and it is my fault too, sorry for that)

I'm +1 for *Documentation Only*
I'm +0.5 for *Combinators*

FYI in "library" subdirectory of ghc source tree I found 3 uses of
uninterruptibleMask. Core libraries deal with low level detail often,
but still only 3 uses, I'm surprised. Is it enough for default behavior.

They are:
 - base/Control/Concurrent/QSem.hs
 - base/Control/Concurrent/QSemN.hs
 - base/System/Timeout.hs

If there are places where uninterruptibleMask is missing in core
libraries, then we should fix this bugs and then decide whether it is
used frequently enough to switch defaults.

On Fri, 2014-11-14 at 13:30 -0500, 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.
> >
> > [1] -
> > 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
> >
> _______________________________________________
> Libraries mailing list
> Libraries at haskell.org
> http://www.haskell.org/mailman/listinfo/libraries