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

On Thu, 2014-11-13 at 12:18 +0200, Eyal Lotem wrote:

On Thu, Nov 13, 2014 at 11:46 AM, Yuras Shumovich wrote:

...

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.

Ok, lets fix all other cases too.

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.

You probably have some special requirements, but that code is bad in general case. It doesn't guaranty anything because the process may never exit. Here you probably should try to terminate it (and close standard streams if any) and then wait for it. (Unless the process is short living *and* doesn't communicate with parent.) Anyway, waitForProcess is not a cleanup action and was not designed for that. (It is hard to define general cleanup action for process because it depends on use case.) You should consider it's contract before using in cleanup action.

hClose, waitForProcess *and any other operation* that is used as a cleanup needs to avoid being interrupted by async exceptions.

No, but they should be prepared for that. For example, hClose can block for minutes (hours, days -- depends of system configuration) if the file is on NFS or it if a network socket because it tries to flush buffers. And you'll not be able to interrupt it if you add uninterruptibleMask in bracket. The alternative is to wrap takeMVar into uninterruptibleMask, in that case hClose becomes exception safe and interruptible.

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

Well, I can't agree with that. As I stated it the first email, the proposal hides real issues. It that case -- it hides the fact that a code is not (sync) exception safe. I'm sure cleanup action should be exception safe, and async exceptions unhide a lot if such bugs.

...

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.

They are relevant for me, sorry. And making cleanups correct w.r.t. async exceptions is even harder, but it should be done too.

...

Thanks, Yuras