Final bikeshedding call: Fixing Control.Exception.bracket

[Eyal Lotem]

On Thu, Nov 13, 2014 at 11:46 AM, Yuras Shumovich <shumovichy at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/libraries/attachments/20141113/7bd3f32d/attachment.html>