Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

On Thu, 2014-11-13 at 15:51 +0200, Eyal Lotem wrote:
> On Thu, Nov 13, 2014 at 1:21 PM, Yuras Shumovich <shumovichy at gmail.com>
> wrote:
> > You probably have some special requirements, but that code is bad in
> > general case. It doesn't guaranty anything because the process may never
> > exit. Here you probably should try to terminate it (and close standard
> > streams if any) and then wait for it. (Unless the process is short
> > living *and* doesn't communicate with parent.)
> >
> 
> That is true - and indeed my code does actually have a terminateProcess
> <https://github.com/ElastiLotem/buildsome/blob/master/src/Lib/Process.hs#L21-L32>
> in there right before waiting for the process (and more stuff too).

Hmm, at the first glance, the code looks overengineered. I wonder
whether it is correct even under uninterruptibleMask. I may miss the
point though, but I'm not surprised you had hard times with it.

> 
> And even if the process decides never to terminate, it is reasonable for
> the thread that represents it to never terminate just as well.

It may be reasonable in some cases, but it can be totally wrong in other
cases. And there can be code that relies on current behavior.

> 
> 
> >
> > Anyway, waitForProcess is not a cleanup action and was not designed for
> > that. (It is hard to define general cleanup action for process because
> > it depends on use case.) You should consider it's contract before using
> > in cleanup action.
> >
> 
> Which, like all cleanup handlers, consists first and foremost of wrapping
> it with uninterruptibleMask.

No, it doesn't require unconditional wrapping into uninterruptibleMask.
And hClose it an example of that.

> Then, you can also try to ask whether it has
> any problematic sync exceptions you want to handle in some way - and do
> that too (independently).
> 
> 
> >
> > >
> > > hClose, waitForProcess *and any other operation* that is used as a
> > cleanup
> > > needs to avoid being interrupted by async exceptions.
> >
> > No, but they should be prepared for that. For example, hClose can block
> > for minutes (hours, days -- depends of system configuration) if the file
> > is on NFS or it if a network socket because it tries to flush buffers.
> > And you'll not be able to interrupt it if you add uninterruptibleMask in
> > bracket.
> >
> 
> And you don't want to interrupt it because that would be worse than
> blocking -- it would just lose the side effects of closing that you're
> after.

Yes, it will lose side effect, but in most cases it is ok (or even
required). Consider the example:

  withFile "some/path" AppendMode $ \h ->
    hPutStrLn h "hello"

Here side effect of hPutStrLn can be lost because async exception
prevented hClose from flushing buffers. But obviously async exception
can interrupt hPutStrLn itself or even be raised before hPutStrLn (it is
not even masked!). And, again, some code can rely on that.

> 
> 
> > The alternative is to wrap takeMVar into uninterruptibleMask, in that
> > case hClose becomes exception safe and interruptible.
> >
> 
> When you interrupt it, does it close or not? Does it flush the buffers or
> not?

I'll point to source code again:
https://github.com/ghc/ghc/blob/805ee118b823f271dfd8036d35b15eb3454a95ad/libraries/base/GHC/IO/Handle/Internals.hs#L734
Note, that flushWriteBuffer is wrapped into trymaybe, so if it is
interrupted, hClose will catch exception, call hClose_handle_, and
rethrow the exception.
So, the buffer will not be flushed, but underlying IODevice will be
closed.

(In case you are asking for current hClose behavior when it is
interrupted on takeMVar: it simply returns braking it's contract: "If
hClose fails for any reason, any further operations (apart from hClose)
on the handle will still fail as if hdl had been successfully closed")

> For example, your "hClose h1
> `finally` hClose h2" code which is "sync-correct" but still async-incorrect.

Just because of bug in hClose? Come on :)


> > And making cleanups correct w.r.t. async exceptions is even harder, but
> > it should be done too.
> >
> 
> For cleanup handlers, correctness w.r.t async exceptions is to postpone
> them until after the cleanup is complete.
> 

Thanks,
Yuras