On Thu, Nov 13, 2014 at 1:21 PM, Yuras Shumovich <shumovichy@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@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 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