Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

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.

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

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.

Thanks,
Yuras