Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

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.

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.


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

Or wrap our main into uninterruptibleMask :)

main :: IO ()
main = uninterruptibleMask_ $ do
  ....

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

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.