Final bikeshedding call: Fixing Control.Exception.bracket

Sun Nov 16 21:26:49 UTC 2014

On Fri, Nov 14, 2014 at 10:22 PM, Yuras Shumovich <shumovichy at gmail.com>
wrote:

>
> 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.
>

Note that for the non-interruptible cases, either would work, so you want
to measure the frequency of the brackets that have interruptible cleanups.

I think after adding the uninterruptibleMasks to all the needed brackets,
and measuring their percentage of all brackets, you'll get a percentage
somewhere around 100% :-)

>
> 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
>
>
> _______________________________________________
> Libraries mailing list
> Libraries at haskell.org
> http://www.haskell.org/mailman/listinfo/libraries
>

-- 
Eyal
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/libraries/attachments/20141116/708a6469/attachment.html>