Final bikeshedding call: Fixing Control.Exception.bracket

[Yuras Shumovich]

On Thu, 2014-11-13 at 00:43 +0200, Eyal Lotem wrote:
> >
> > -- | database that uses two files
> > data DB = DB Handle Handle
> >
> > closeDB :: DB -> IO ()
> > closeDB (DB h1 h2) = hClose h1 >> hClose h2
> >
> > The cleanup action "closeDB" above is buggy because the first hClose can
> > be interrupted. In that case the first handle will be closed, but the
> > second will leak. Note: "closeDB" is not atomic -- it consists from two
> > interruptible different actions. The same with hClose itself -- if can
> > be interrupted somewhere in the middle, but it is able to handle that.
> >
> > The correct cleanup probably should look like the next:
> >
> > closeDB (DB h1 h2) = hClose h1 `finally` hClose h2
> >
> > Note: the initial version is buggy with respect to both async and sync
> > exceptions, and uninterruptibleMask will fix it only with respect to
> > async exceptions.
> >
> > The second version is (I hope) exception-safe -- it handle both async
> > and sync exceptions. That is important point -- if you need
> > uninterruptibleMask, then probably you have issue with sync exceptions
> > too. Lets fix the original issue and make code exception safe instead of
> > hiding it behind uninterruptibleMask.
> >
> 
> Your second version is not exception-safe: async exceptions would just
> cease the first close, leak the handle and continue to the second close to
> potentially block again and either leak or close the second handle. This is
> not reasonable behavior for cleanup. If you wrap it all with
> uninterruptibleMask then it becomes as correct a cleanup as it can be.

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:

> > It is already implemented in such the way. Let me explain.
> > > There are two sources of possible interruptions in hClose:
> > >   a) takeMVar
> > >   b) flushing internal buffer
> > >
> > > a) is not an issue in practice -- it will not be interrupted
unless
> > > someone already uses the Handle (if it is the case, then you
probably
> > > has bigger issue -- you may use already closed handle.) But it
probably
> > > should be more careful and use uninterruptibleMask here... I don't
have
> > > strong opinion.
> > > b) is handled correctly, see
> > >
> >
https://github.com/ghc/ghc/blob/805ee118b823f271dfd8036d35b15eb3454a95ad/libraries/base/GHC/IO/Handle/Internals.hs#L734
> > > Basically it catches all exceptions (including async,) closes the
handle
> > > and rethrows the exception.
> 

"closes the handle" here means that "close" method of underlying
IODevice is called. And now it is IODevice's author responsibility to
handle exceptions correctly.

> 
> Sync exceptions when closing your DB handles leave it in an undefined state
> (or at least, that's the underlying behavior of POSIX close). At that
> point, the handles cannot be re-closed (since they may have been reused in
> a different context).
> 
> 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.

> 
> In my opinion you're incorrectly equating sync and async exceptions. The
> former can only be avoided by satisfying preconditions which you must do in
> cleanups. The latter can only be avoided by uninterruptibleMask, which you
> must also do in cleanups. The combination of the two is the only way to
> make exception-safe cleanups that:

I'm not equating them, I'm arguing for exception safe code.
Don't lie yourself, hClose can throw sync exception and it *will* throw
it sooner or later. If you are not prepared for that, you'll get
mysterious bug. But if you are prepared, then just don't need
uninterruptibleMask in bracket.

> 
> A) Do not break invariants if async exception is sent during cleanup
> B) Do not cause an exception within an exception (e.g: during bracket,
> onException or finally) where at least one exception must be lost, which is
> yet another bug which was overlooked in this discussion

It is not overlooked (I even posted link to discussion of this issue in
the my fist reply to the thread.) But it is simply not relevant.


> If hClose throws a sync exception there's *nothing* that can be done to
> make the code not leak the resource.
> 
> However:
> 
> bracket openFile hClose -- is correct with uninterruptibleMask and
> incorrect with mask. The potential for a sync exception in hClose here is
> irrelevant to the correctness that can be attained.
> 
> So it *does* in fact make writing exception-safe code much much easier.

Could you please point me to line in source code where hClose can throw
exception without calling IODevice.close? Where it can be interrupted by
async exception? And if you find such places, then why should not it be
fixed?


If you find that uninterruptibleMask makes your life easer, then go
ahead and use it. Sometimes it is even necessary to make code exception
safe. But it is bad idea to use it in bracket from base because it
actually only hides bug, not fixes them. As a result more bugs will
remain unnoticed and not fixed for longer period.

Thanks,
Yuras