Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

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

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.

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

The alternative is to wrap takeMVar into uninterruptibleMask, in that
case hClose becomes exception safe and interruptible.

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

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

> 
> 
> >
> > Thanks,
> > Yuras
> >
> >
> >
> 
>