Final bikeshedding call: Fixing Control.Exception.bracket

[Eyal Lotem]

On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich <shumovichy at gmail.com>
wrote:

> On Thu, 2014-11-13 at 21:06 +0000, John Lato wrote:
> >  On 12:31, Thu, Nov 13, 2014 Yuras Shumovich <shumovichy at gmail.com>
> wrote:
> >
> >
> > TLDR: the proposal is still a bad idea :))))))
> >
> >
> > IMHO it's better than the current situation.
> >
> > On Thu, 2014-11-13 at 18:20 +0000, John Lato wrote:
> > >  hClose is a good example because it's pervasive and currently
> incorrect.
> > > But it's not the only one, and furthermore a lot of broken code is
> likely
> > > in libraries.
> >
> > hClose is pervasive, but it doesn't mean the bug in it affects a lot of
> > code. Probably 99.(9)% hClose uses are correct. (it may affect more code
> > then I think, but nobody even created a ticket yet!) Do you really think
> > it is common to close handle while using it from other thread?
> >
> >  No, but that's not a general requirement for this problem to manifest.
>
> Sorry, I didn't get it. You mean concurrent access to handle is not a
> general requirement for the bug in hClose to manifest itself? There are
> other cases where hClose leaks file descriptor?
>
> > Besides, why shouldn't one be able to safely close a handle while another
> > thread may be using it, without using arcane functions?
>
> We *should* be able. So lets fix hClose.
>
> >
> > But yes, there is a lot of broken code in libraries. But we should fix
> > bugs instead if hiding them.
> >
> >  This isn't about hiding bugs. It's about changing the semantics of
> > exception handlers so that they are more likely to match what's desired
> in
> > the general case.
>
> This *is* about hiding bugs. I believe that in most cases async
> exceptions uncover bugs that are not actually async-only. So using
> uninterruptibleMask you are hiding real bug. And I already gave an
> example, I'll reproduce it here:
>
> data DB = DB Handle Handle
>
> closeDB :: DB -> IO ()
> closeDB (DB h1 h2) = hClose h1 >> hClose h2
>
> Here closeDB is buggy with respect to both sync and async exceptions.
> Fixing it is trivial:
>
> -- here I assume that the bug in hClose is fixed
> closeDB (DB h1 h2) = hClose h1 `finally` hClose h2
>
> That fixes it w.r.t. sync *and* async exception without any special work
> for async case.
>

NO: This new code is still broken. Async exception in close of h1 will just
"jump" to block at h2, and yield the full side effect of closing h2 while
skipping some of the side effect of closing h1.

All my code used these kind of idioms for sync-exception-safety but was
still ridden with bugs w.r.t async exceptions.

The kill&wait for process are another example, but I have multiple examples
-- all of which become more reasonable when the cancellation is
uninterruptible.

>
> Almost nobody test code for case when file is deleted, so it is unlikely
> to discover the bug in the first version in case of uninterruptibleMask.
> So the proposal *hides* the bug, probably for 99% cases, but the bug is
> here, and it will bit you sooner or later.
>

Are you talking about Windows or POSIX? With POSIX, file deletion has
nothing to do with hClose.


>
>
> > The proposal makes it easer to continue ignoring async exceptions. That
> > is why I'm arguing here against it. (Possible breakage if existing code
> > worries me too, but much less)
> >
> >  I think it's a good thing to make it easier to ignore async exceptions.
>
> It is already easy -- just don't use them.
>

Whenever you use the async library, for example, you use async exceptions.
And then all your bracket invariants are broken *by-default*.


> Or wrap our main into uninterruptibleMask :)
>
> main :: IO ()
> main = uninterruptibleMask_ $ do
>   ....
>

But cancelling/killing non-cleanups is not problematic in general.


>
> >
> > It is common myth that bracket saves you from async exceptions, but that
> > is simply not true. And the proposal will not make it true. So newcomers
> > learn to ignore async exceptions and continue doing that forever.
> >
> >  Even worse, when newcomer finally find out (most likely itself!!!), that
> > the myth is wrong, he doesn't get help from the community -- no docs, no
> > tutorials, no blogs. Everything he can find is a set of myths.
> >
> >  Or we could make the implementation match the common knowledge. Then it
> > wouldn't be a myth, it would be true.
>
> No, it will *not* be true. Did you read the article I mentioned?
>
> >
> >
> > I started learning haskell 8 years ago, and I'm paid for haskell code 3
> > years already. When do you think I discovered that bracket is not enough
> > to handle async exceptions? Half a year ago(!)
> >
> > I'll post the link again:
> >
> http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5752251.html
> > Note that nobody answered the question about hClose. Nobody explained
> > how to use interruptible exceptions in cleanup. Very few people actually
> > even care to replay. Because everybody learned to ignore async
> > exceptions.
> >
> > Did you read this: http://www.well-typed.com/blog/97/
> > ?
> > I hope you did. Because it is probably the *only* deep discussion of
> > *some* of the related issues. Unfortunately it appears after too late
> > for me, so I spent a lot of days discovering everything myself.
> >
> >  Wouldn't it be better if stuff like that just worked out of the box?
> > People could still spend days learning it, but they wouldn't be bitten by
> > these hard-to-identify, poorly-understood bugs.
> >
> > I even wrote my own library for exception handling:
> > https://github.com/Yuras/io-region/
> > I'm still not sure it is good. It even can be buggy. But I considered a
> > lot of design decisions, including unintrruptibleMask, and found then
> > unsatisfactory
> >
> > .
> >
> > I personally find it easer to write exception safe code with io-region,
> > but you should understand async exceptions anyway. There is no easy way
> > unfortunately.
> >
> >  That doesn't mean we should make it harder to do the right thing.
>
> The proposal doesn't make it significantly easer. It just hides bugs.
>

The current situation hides bugs -- virtually all uses of bracket in the
wild leak resources and break invariants in their cancellation if async
exceptions are thrown, whereas with the proposal they all become correct
w.r.t async exceptions modulu deadlock concerns.


>
> It is a myth that handling async exceptions in cleanup is much harder
> then handling sync exceptions. Async exceptions are hard to deal with
> because they can be raised at any point (even between points :) ). But
> in cleanup action async exceptions are masked, and can be raised only in
> well known points, just like regular sync exceptions.
>

Sync exceptions are under your control. You can make sure the preconditions
of the operation are met so that they are not raised. If they are raised,
they are related to the operation at hand so there may be something that
you can do.  Async exceptions are fundamentally different, so please stop
mishmashing these two dissimilar things together.



-- 
Eyal
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/libraries/attachments/20141114/20bfcb9f/attachment-0001.html>