Re: Final bikeshedding call: Fixing Control.Exception.bracket

13 Nov 2014


      On Fri, 2014-11-14 at 01:54 +0200, Eyal Lotem wrote:
...
On Fri, Nov 14, 2014 at 1:00 AM, Yuras Shumovich 
wrote:
...
On Fri, 2014-11-14 at 00:33 +0200, Eyal Lotem wrote:
...
On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich 
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.

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