Final bikeshedding call: Fixing Control.Exception.bracket

[Eyal Lotem]

On Thu, Nov 13, 2014 at 1:21 PM, Yuras Shumovich <shumovichy at gmail.com>
wrote:

> On Thu, 2014-11-13 at 12:18 +0200, Eyal Lotem wrote:
> > On Thu, Nov 13, 2014 at 11:46 AM, Yuras Shumovich <shumovichy at gmail.com>
> > wrote:
> > > Ok, your example convinced me that it may be an issue in practice. But
> > > I'm still not sure it *is* an issue in practice because nobody created
> > > an issue about that.
> > > Note: there were an issue (see here:
> > > https://ghc.haskell.org/trac/ghc/ticket/3128 ) about resource leak
> > > caused by sync exception inside hClose, but it was fixed.
> > >
> > > Anyway, lets fix hClose, not bracket.
> > >
> >
> > But hClose is only an example.
>
> Ok, lets fix all other cases too.
>
> >
> > One of the problematic cases in my code was actually in the lines of:
> >
> > bracket (createProcess ..) (waitForProcess ..)
> >
> > This code *must* guarantee the invariant that the process no longer
> exists
> > when it leaves. Async exceptions should not violate this.
>
> 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).

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


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


> 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 agree with Merijn
> > that bringing sync exceptions into this is irrelevant, and there is
> really
> > no such thing as sync-exception safety for a cleanup handler anyway (the
> > cleanup won't happen, that is a bug!).
>
> Well, I can't agree with that. As I stated it the first email, the
> proposal hides real issues. It that case -- it hides the fact that a
> code is not (sync) exception safe. I'm sure cleanup action should be
> exception safe, and async exceptions unhide a lot if such bugs.
>

No, the code can be sync-exception-safe (as far as possible, anyway) but
async exceptions will still wreck havoc. For example, your "hClose h1
`finally` hClose h2" code which is "sync-correct" but still async-incorrect.

>
> > >
> > > Note: users already relies on hClose doesn't leak in case of exception
> > > (see Trac #3128 I pointed to above) (hClose does try to guaranty that).
> > > So we can't just ban sync exceptions in cleanup without deeper
> > > investigation.
> > >
> >
> > Sync exceptions are irrelevant. Making cleanups correct w.r.t sync
> > exceptions is not easy and should be done, but is a different problem
> than
> > what this proposal is about.
>
> They are relevant for me, sorry.
>
> 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
> > >
> > >
> > >
> >
> >
>
>
>


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