Final bikeshedding call: Fixing Control.Exception.bracket

[Eyal Lotem]

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

> On Fri, 2014-11-14 at 01:54 +0200, Eyal Lotem wrote:
> > On Fri, Nov 14, 2014 at 1:00 AM, Yuras Shumovich <shumovichy at gmail.com>
> > wrote:
> >
> > > On Fri, 2014-11-14 at 00:33 +0200, Eyal Lotem wrote:
> > > > On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich <
> shumovichy at gmail.com>
> > > > wrote:
> > > > > 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.
> > >
> > > Yes, but that is good default, I already explained why. If you care
> > > about about side effect in case of async exception, when use
> > > uninterruptibleMask explicitly, and explain in comments why you need
> > > that.
> > >
> > I disagree that it is a good default. "withFile" combined with successful
> > writes to the handle now no longer guarantee the writes occur. Your
> writes
> > get lost forever despite diligently using the correct idioms, whenever
> > async exceptions are involved.
>
> You are missing the point. Async exception can interrupt "withFile" in
> acquire, cleanup or body, and you can't rely on any particular case. So
> there should not be any difference whether write to the handle was lost
> because of hPutStr was interrupted, or because hClose failed to flush
> buffers. withFile doesn't provide atomic guaranties.
>

You misunderstand me. With your proposal for hClose and current bracket,
users would no longer have any useful guarantee about *success* results
from writes to handles *inside* the withFile bracket. But I think the
entire discussion focused too much on hClose whereas other examples make
bracket's terrible default much clearer.


>
> But you can propose to change the default behavior of hClose if you find
> it problematic. It is not relevant here.
>
> >
> > I argue that it is *very* rare that your cleanups have anything sensible
> to
> > do upon async exceptions (I don't think I've ever encountered a single
> case
> > where that was desirable) and yet it is very common for cleanups to
> > "forget" to uninterruptible-mask when they should (almost always). Again,
> > this comes up when you review random uses of bracket, which are broken in
> > virtually every case of an interruptible cleanup.
> >
> > The sane default is to guarantee that the cleanup happens even if there
> is
> > an async exception. Then someone who has some sensible thing to do when
> an
> > async exception occurs during cleanup can go ahead and use the
> > rarely-useful bracketInterruptible.
> >
> >
> > > >
> > > > All my code used these kind of idioms for sync-exception-safety but
> was
> > > > still ridden with bugs w.r.t async exceptions.
> > >
> > > Several options:
> > > a) your code is buggy, fix it
> > > b) you rely on buggy code, pester it's author to fix it (and temporary
> > > use uninterruptibleMask)
> > > c) you really need uninterruptibleMask here, go ahead and use it
> > >
> > >
> > Why don't we consider some realistic examples?
> >
> > withFile => you want it to flush successful writes => You want
> > uninterruptibleMask.
>
> No, I don't, see above.
>
> > withMVar => you want it to guarantee the MVar doesn't remain in an
> > inconsistent state => You want uninterruptibleMask
>
> I don't want it to block indefinitely if I incidentally put something
> into the mvar somewhere else.


You want to go to an incoherent program state, instead?!


> If you don't manipulate the mvar yourself
> (and you shouldn't actually), then withMVar does not leave the mvar in
> inconsistent state. I don't want uninteruptibleMask here.
>

It is entirely plausible to have a program where the MVar contains some
resource, and some background thread fills that mvar from some resource
pool and eventually restores a resource to the pool (bracket putMVar
takeMVar) - and another thread pulls the resource from the mvar to use it
and eventually puts the resource back into the mvar.

i.e: you have concurrent reversed brackets:

bracket takeMVar putMVar
bracket putMVar takeMVar

NOTE: Only in such situations the cleanup here may be interruptible at all.

Of course any interruptible mvar operation will *not* occur if it is
interrupted. Of course we want our cleanup to not break our mvar's
invariants. So of course any cleanup involving interruptible mvar
operations will want these operations *not to be* interruptible during
cleanup.


> > createProcess/kill+wait => you want to guarantee that the process-bracket
> > actually maintains an invariant about the process's state => you want
> > uninterruptibleMask
>
> I don't want it to block for unbound amount of time. I don't want
> uninteruptibleMask here.
>

You prefer it to break program invariants?

I don't understand how you could possible prefer random breakage over long
blocking.

Not to mention: If you don't care that the process is actually terminated
in an event of an exception -- why would you use bracket at all?

If I put the "terminateProcess `finally` waitForProcess" in my bracket
cleanup -- it doesn't say: "Please clean up this process on a good day". It
is saying: "My program must not have this process running by the time this
bracket completes".

I think if you disagree on this -- then we simply have a completely
different understanding of what "bracket" is meant to achieve in the first
place!


>
> > withSemaphore => you want to guarantee that the semaphore doesn't remain
> in
> > an inconsistent state => you want uninterruptibleMask
>
> I don't remember anything about it, but I'm not sure I'd want
> uninterruptibleMask here.
>

Again, if you prefer to prevent unbounded blocking at the expense of
randomly breaking program invariants -- of course you don't want it here.

But if you agree that upholding program invariants is a basic prerequisite
for program correctness, then you cannot possibly hold this position.


>
> > See a pattern here?
>
> Yes, but it probably differs from your's :)
>
> >
> >
> > > >
> > > > The kill&wait for process are another example, but I have multiple
> > > examples
> > > > -- all of which become more reasonable when the cancellation is
> > > > uninterruptible.
> > >
> > > Then just use uninterruptibleMask if it is reasonable in your 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.
> > > > >
> > > >
> > > > Are you talking about Windows or POSIX? With POSIX, file deletion has
> > > > nothing to do with hClose.
> > >
> > > Ok, Almost nobody test code for case when <insert a case when your
> > > cleanup action throws sync exception>
> > >
> > > sync exceptions are irrelevant here.
>
> Ok. Sync exceptions are irrelevant here. For you.
>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > > 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*.
> > >
> > > Don't use async then.
> > >
> >
> > Whereas with this proposal, people can safely use async and bracket will
> > work correctly.
> >
> >
> > > >
> > > >
> > > > > Or wrap our main into uninterruptibleMask :)
> > > > >
> > > > > main :: IO ()
> > > > > main = uninterruptibleMask_ $ do
> > > > >   ....
> > > > >
> > > >
> > > > But cancelling/killing non-cleanups is not problematic in general.
> > >
> > > Did you read "Dealing with Asynchronous Exceptions during Resource
> > > Acquisition" article?
> > >
> >
> > Yes.
> > I didn't say it's not tricky -- I said it's not problematic. i.e: It's
> very
> > possible to make resource acquisition cancelable - and it is often
> critical
> > that we can abort threads that are still at resource acquisition stage.
>
> You didn't recognized symmetry between acquire and cleanup? I even can
> replicate similar bug as in hClose, but in acquire. I can't understand
> why you find cleanup problematic, but acquire -- not.
>

Because exceptions during an IO action are supposed to:
A) prevent the side-effects of that IO action from occuring, when possible
A.2) avoid leaking any partially-allocated resources

These general IO action requirements make it safe to abort a bracket
allocation, as if the bracket was never entered in the first place.

However, for a cleanup -- these do not help! In a cleanup, "abort" is
virtually always not an option, unless again you are willing to throw away
your program invariants.

This is why the two are not symmetric, and letting the allocation be
interrupted as long as it is a well-founded IO action is fine, whereas for
cleanup an abort is simply going to wreak havoc in almost every case.


> >
> >
> > >
> > > > >
> > > > > 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.
> > > >
> > >
> > > So you want to ban sync exceptions in cleanup actions?
> > >
> > >
> > What?? No.
> >
> > I want to write my code in such a way that eliminates as many sync
> > exceptions as possible. For example, I will avoid passing invalid handles
> > to hClose - and I have a guarantee that one sync exception will not
> happen.
>
> Ok, but what about exceptions that you can't eliminate? You either ban
> them or handle them. If you handle them, then goto "It is a myth that
> handling async exceptions in cleanup is much harder then handling sync
> exceptions"
>
>
>
There is no real way to "handle" most sync exceptions in cleanups. For
example, your "handling" of exceptions in the double-close example does not
"handle" the exception at all, it just contains the damage done (failure to
close the first handle) to the first handle, but the damage was done.

If a cleanup fails with a sync exception, either you retry, or you failed
to clean up and you've leaked a resource or broke an invariant. In some
cases you could try to escalate the cleanup to a higher level where it is
still possible, but that requires system-wide design around this and cannot
be done in a simple exception handler.

Thus, sync exceptions are in a sense a lost cause when it comes to robustly
handling them, at least locally. Just do your best to prevent them from
happening -- and indeed contain their damage as much as possible (as you
did by using `finally` to continue the cleanups you still can salvage).

Async exceptions, however, are possible to robustly prevent. If you let
them interrupt your cleanup and then try to handle them -- you've already
lost the cleanup work that was supposed to happen, and the game is over, it
is too late.

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