On Thu, Nov 13, 2014 at 11:46 AM, Yuras Shumovich <shumovichy@gmail.com> wrote:

On Thu, 2014-11-13 at 02:28 +0000, John Lato wrote:
> On Thu Nov 13 2014 at 8:58:12 AM Yuras Shumovich <shumovichy@gmail.com>
> wrote:
> > You are wrong, hClose closes the handle in case of any exception, so
> > there is no leak here. I already described that and pointed to source
> > code. Probably my arguments are weak, but nobody even tried to argue the
> > opposite. The relevant part:
> >
>
> People have been arguing the opposite. hClose is not guaranteed to close
> the handle in case an exception arises. Here's a demonstration program.

Sorry, I mean that nobody argued that my analysis of hClose behavior is
wrong. I noted that hClose can leak in case of concurrent access to the
handle, but found that not an issue in practice. Until now nobody argued
the opposite.

> You might argue that hClose should use uninterruptibleMask internally
> (which is the only way to fix the issue). Possibly so. However, this is a
> really pervasive problem, which is why it makes some sense to place the
> mask in bracket and fix every handler properly.

Yes, I argue that hClose should be fixed, not bracket. I don't have
strong opinion whether it should use uninterruptibleMask or handle it in
any other way.

>
> At some point in this thread a person (you?) has argued that this isn't a
> problem in practice. I disagree. It actually seems to be fairly common in
> certain types of network programming.

(You are correct, it was me)

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.

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.

hClose, waitForProcess and any other operation that is used as a cleanup needs to avoid being interrupted by async exceptions. 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!).

> > > So sync exceptions in hClose mean the program is incorrect, and the only
> > > recourse is to prevent the sync exceptions in the first place.
> > Fortunately,
> > > these FDs are likely guaranteed to be valid so sync exceptions are
> > > virtually ruled out.
> > >
> > > This is a general pattern with cleanups: a cleanup already has the
> > > allocated resource at hand, which almost always rules out sync
> > exceptions.
> > > Also, exceptions during an error-induced cleanup cause dangerous
> > > error-silencing anyway since we cannot handle an exception within an
> > > exception.
> >
> > So you have to inspect all the code, directly or indirectly used by
> > cleanup action, to ensure it doesn't throw sync exception (just to find
> > that it is not the case -- a lot of cleanup actions can throw sync
> > exceptions in some, probably rare, cases.) Someone argued, that was
> > exactly the issue the proposal was trying to solve.
> >
>
> Sync exceptions have nothing to do with the proposal. The proposal itself
> certainly doesn't argue this.

Sorry, I was not clear enough. I'm referring to the next:

On Tue, 2014-11-11 at 12:17 -0800, Merijn Verstraaten wrote:
> Both Eyal and me have had trouble with this where we had to entire
> half of base and part of the runtime, to figure out whether our code
> was async exception safe. Auditing half the ecosystem to be able to
> write a safe cleanup handler is *NOT* a viable option.

By banning sync exceptions in cleanup action you require Merijn to audit
half the ecosystem to figure out whether his code is *sync* exception
safe. Which probably requires the same amount of work as inspecting code
for *async* exception safety.

But the inspection is really a red herring. Your inspection will have one of two results:

A) The code is interruptible -- preventing the cleanup from completing when async exception exists -> Need to wrap it with uninterruptibleMask

B) The code is not interruptible -- it doesn't matter at all whether it is wrapped with uninterruptibleMask, this whole discussion is moot.

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.

Thanks,
Yuras

Eyal