Re: Final bikeshedding call: Fixing Control.Exception.bracket

On 01:46, Thu, Nov 13, 2014 Yuras Shumovich 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 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. hClose is a good example because it's pervasive and currently incorrect. But it's not the only one, and furthermore a lot of broken code is likely in libraries. For the most part, Haskellers don't need to worry about async exceptions. using them properly is a rather rare skill currently, and I suspect people are basically happy with that. But it's dangerous because people actually do need to write async-safe code in cleanup handlers to get the behavior that usually mean. One reason I think changing bracket etc is a better solution is that it helps minimize the average programmer's exposure to async exceptions. It becomes easier to write correct cleanup handlers, for everyone. And if someone really needs an interruptible handler that can be available by a specialized function. But bracket and catches aren't the place for that.

...

...

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. What? Nobody wants to ban sync exceptions in cleanups. They should continue to work the way they do now. Why do you bring this up?