Just for clarification, what's up for discussion at this point? Is everyone at least convinced that the status quo is untenable, and we're trying to achieve consensus (or at least a mandate) on a solution? As for current solutions, I think #2 is best. Functions like bracket need to "just work" in general. On Tue Nov 11 2014 at 10:18:50 AM David Feuer wrote:

I like #2. Errors you can detect easily are much better than errors that silently mess things up. On Nov 11, 2014 1:10 PM, "Merijn Verstraaten" wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y))) This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup". I disagree that it should be left to the author of allocation operation to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler. 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. Cheers, Merijn

On 11 Nov 2014, at 11:58, Yuras Shumovich wrote:

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5... ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use `uninterrubtibleMask` internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

The flaw in your argument is that you're assuming that the author of a cleanup action is a single library author, rather than a random user using bracket with a random action from library X as cleanup. Now the user has to investigate whether random action X can block. There's not a single library that properly documents "this action can potentially block" because in many cases even the library authors themselves aren't sure. Not to mention, even if library authors *did* document this, then this would be incredibly brittle as the correctness of my code now relies on my recursive dependencies all properly documenting blocking behaviour AND following the PVP to avoid accidental breakage. This is just not realistic, the only sane option is to fix it unambiguously in bracket so I as a user can safely assume that my cleanup is never interrupted. Unless you can provide me with a concrete, *real*, actual library that requires cleanup to be interruptible, I will not believe this line of arguing. -- Merijn On Tue, Nov 11, 2014, at 13:00, Yuras Shumovich wrote:

On Tue, 2014-11-11 at 12:17 -0800, Merijn Verstraaten wrote:

...

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

It definitely can be done for cleanups too. E.g. using uninterruptibleMask as the last resort.

And "half a cleanup" is valid thing. E.g. database that uses two files:

data DB = DB Handle Handle

The following cleanup action is buggy:

close :: DB -> IO () close (DB h1 h2) = hClose h1 >> hClose h2

Note: it is broken regardless async exceptions(!) You can get half cleanup even under uninterruptibleMask

The code is either exception-safe or buggy. You can't magically fix buddy code using uninterruptibleMask.

...

I disagree that it should be left to the author of allocation operation to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

(You probably mean "the author of *cleanup* operation"? I'll assume that)

Hm... You have to inspect code if you expect it to be buggy. Otherwise you should assume it is interruptible (unless listed in Cotrol.Exception module in "Interruptible operations" section or explicitly stated in other way), but all cleanup actions do full cleanup even if they throw exception. If the code is buggy -- it should be fixed, not hidden.

...

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.

You need to audit half of ecosystem anyway to ensure allocating actions are not buggy. And as the example above shows, even uninterruptibleMask doesn't guaranty anything.

I agree that current situation is bad. I have two drafts in my mailbox where I propose to use uninterruptibleMask in bracket, but I didn't send them -- every time I found that it doesn't fix anything actually. I don't know better solution, but the proposal is not even a solution.

Thanks, Yuras

...

Cheers, Merijn

...

On 11 Nov 2014, at 11:58, Yuras Shumovich wrote:

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5... ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use `uninterrubtibleMask` internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

I don't follow your argument. You state that a user should assume their cleanup action can block. Blocking implies that the function may receive an async exception. The only way to guarantee code runs in the presence of async exceptions is uninterruptibleMask (or something built on it). So doesn't that assumption imply that uninterruptibleMask should be the default? Although you also seem to be saying that functions like hClose etc. should use uninterruptibleMask internally anyway. Possibly, but I'm less convinced that's a good idea. It's a pretty blunt hammer to have hidden from the user, and if it has to be used bracket seems like a better choice because it will fix many cases instead of just one and library authors won't have to struggle to get it right. If you can write an hClose implementation that does the right thing, is async-safe, and doesn't use uninterruptibleMask or the equivalent, I might be convinced the proposed solution is bad. John L. On 13:03, Tue, Nov 11, 2014 Yuras Shumovich wrote:

On Tue, 2014-11-11 at 12:17 -0800, Merijn Verstraaten wrote:

...

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

It definitely can be done for cleanups too. E.g. using uninterruptibleMask as the last resort.

And "half a cleanup" is valid thing. E.g. database that uses two files:

data DB = DB Handle Handle

The following cleanup action is buggy:

close :: DB -> IO () close (DB h1 h2) = hClose h1 >> hClose h2

Note: it is broken regardless async exceptions(!) You can get half cleanup even under uninterruptibleMask

The code is either exception-safe or buggy. You can't magically fix buddy code using uninterruptibleMask.

...

I disagree that it should be left to the author of allocation operation

to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

(You probably mean "the author of *cleanup* operation"? I'll assume that)

Hm... You have to inspect code if you expect it to be buggy. Otherwise you should assume it is interruptible (unless listed in Cotrol.Exception module in "Interruptible operations" section or explicitly stated in other way), but all cleanup actions do full cleanup even if they throw exception. If the code is buggy -- it should be fixed, not hidden.

...

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.

You need to audit half of ecosystem anyway to ensure allocating actions are not buggy. And as the example above shows, even uninterruptibleMask doesn't guaranty anything.

I agree that current situation is bad. I have two drafts in my mailbox where I propose to use uninterruptibleMask in bracket, but I didn't send them -- every time I found that it doesn't fix anything actually. I don't know better solution, but the proposal is not even a solution.

Thanks, Yuras

...

Cheers, Merijn

...

On 11 Nov 2014, at 11:58, Yuras Shumovich

...

...

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception-

bracket-is-broken-td5752251.html ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

...

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use

`uninterrubtibleMask`

...

internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds

...

...

...

The rest seems either +1, or at least agrees that the status quo is

*worse* than the proposed solution.

...

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup

handler being interruptible

...

2) There are plenty of examples of current code being broken, for

example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally,

...

...

...

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an

uninterruptible cleanup handler.

...

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add

bracketInterruptible for interruptible cleanups.

...

Trade-offs: Solution 1 won't change the semantics of any existing code, however

...

...

...

Solution 2 will change the semantics of bracket, which means any

currently broken uses of bracket will be fixed, at the cost of creating

...

...

...

I will argue that solution #2 is preferable, since I have yet to see

any code that uses the interruptibility of the cleanup handler. Whereas

...

...

...

Secondly, it is easier to detect deadlocks caused by this problem

wrote: that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem". potentially leaking file descriptors/handles. this also means that any currently broken uses of bracket will remain broken, possibly indefinitely. potential deadlocks in code that relies on the interruptibility of cleanup. there's many broken assumption assuming the cleanup handler is not interruptible. than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

...

...

...

[1] - https://www.haskell.org/pipermail/libraries/2014-

September/023675.html

...

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On Wed, 2014-11-12 at 02:02 +0300, Yuras Shumovich wrote:

On Tue, 2014-11-11 at 22:14 +0000, John Lato wrote:

...

I don't follow your argument. You state that a user should assume their cleanup action can block.

Lets use accurate terms. You probably mean "can be interrupted". If action can block, then it is may (usual case) or may not (e.g. when async exceptions are masked) be interruptible. Please let me know if you disagree with this definitions -- I just want to ensure everybody uses the same definitions.

...

Blocking implies that the function may receive an async exception. The only way to guarantee code runs in the presence of async exceptions is uninterruptibleMask (or something built on it).

I don't understand that. The code definitely runs with or without uninterruptibleMask. In the first case it can be interrupted by async exception. But in both cases it can throw sync exception.

Hmm, I reread your email, and now I probably see what you mean. You think about cleanup as atomic action that either runs or is interrupted. But that is not true. Consider the example I already used earlier: -- | 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.

...

So doesn't that assumption imply that uninterruptibleMask should be the default?

No, I don't see how it implies that.

...

Although you also seem to be saying that functions like hClose etc. should use uninterruptibleMask internally anyway. Possibly, but I'm less convinced that's a good idea. It's a pretty blunt hammer to have hidden from the user, and if it has to be used bracket seems like a better choice because it will fix many cases instead of just one and library authors won't have to struggle to get it right.

No, I propose to write exception safe code in the first place. And use uninterruptibleMask if it is necessary (usually it is not necessary.)

Just adding uninterruptibleMask to bracket doesn't make is easer to write exception safe code. hClose can throw (synchronous) exceptions anyway, so you should be prepared. There is no other way except ensuring that hClose is exception-safe *and* you are using it in exception-safe manner.

...

If you can write an hClose implementation that does the right thing, is async-safe, and doesn't use uninterruptibleMask or the equivalent, I might be convinced the proposed solution is bad.

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/lib... Basically it catches all exceptions (including async,) closes the handle and rethrows the exception.

Let me state it again: hClose closes the handle in case of exception, including async exception. And that is the only correct behavior -- every cleanup action should do that, otherwise it is not exception safe. Masking async exception doesn't magically make code exception safe.

...

John L.

On 13:03, Tue, Nov 11, 2014 Yuras Shumovich wrote:

...

On Tue, 2014-11-11 at 12:17 -0800, Merijn Verstraaten wrote:

...

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

It definitely can be done for cleanups too. E.g. using uninterruptibleMask as the last resort.

And "half a cleanup" is valid thing. E.g. database that uses two files:

data DB = DB Handle Handle

The following cleanup action is buggy:

close :: DB -> IO () close (DB h1 h2) = hClose h1 >> hClose h2

Note: it is broken regardless async exceptions(!) You can get half cleanup even under uninterruptibleMask

The code is either exception-safe or buggy. You can't magically fix buddy code using uninterruptibleMask.

...

I disagree that it should be left to the author of allocation operation

to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

(You probably mean "the author of *cleanup* operation"? I'll assume that)

Hm... You have to inspect code if you expect it to be buggy. Otherwise you should assume it is interruptible (unless listed in Cotrol.Exception module in "Interruptible operations" section or explicitly stated in other way), but all cleanup actions do full cleanup even if they throw exception. If the code is buggy -- it should be fixed, not hidden.

...

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.

You need to audit half of ecosystem anyway to ensure allocating actions are not buggy. And as the example above shows, even uninterruptibleMask doesn't guaranty anything.

I agree that current situation is bad. I have two drafts in my mailbox where I propose to use uninterruptibleMask in bracket, but I didn't send them -- every time I found that it doesn't fix anything actually. I don't know better solution, but the proposal is not even a solution.

Thanks, Yuras

...

Cheers, Merijn

...

On 11 Nov 2014, at 11:58, Yuras Shumovich

...

...

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception-

bracket-is-broken-td5752251.html ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

...

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use

`uninterrubtibleMask`

...

internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds

...

...

...

The rest seems either +1, or at least agrees that the status quo is

*worse* than the proposed solution.

...

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup

handler being interruptible

...

2) There are plenty of examples of current code being broken, for

example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally,

...

...

...

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an

uninterruptible cleanup handler.

...

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add

bracketInterruptible for interruptible cleanups.

...

Trade-offs: Solution 1 won't change the semantics of any existing code, however

...

...

...

Solution 2 will change the semantics of bracket, which means any

currently broken uses of bracket will be fixed, at the cost of creating

...

...

...

I will argue that solution #2 is preferable, since I have yet to see

any code that uses the interruptibility of the cleanup handler. Whereas

...

...

...

Secondly, it is easier to detect deadlocks caused by this problem

wrote: that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem". potentially leaking file descriptors/handles. this also means that any currently broken uses of bracket will remain broken, possibly indefinitely. potential deadlocks in code that relies on the interruptibility of cleanup. there's many broken assumption assuming the cleanup handler is not interruptible. than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

...

...

...

[1] - https://www.haskell.org/pipermail/libraries/2014-

September/023675.html

...

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

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.

...

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

...

...

has bigger issue -- you may use already closed handle.) But it

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: probably 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/lib...

...

...

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

First let's have a discussion about whether or not that program *should* have worked as written. It isn't immediately obvious that Handles should be closed out from under other threads that are using them like that. I certainly try to avoid it. For example, we could modify the program like this so that the thread was responsible for cleaning up its own resources. (On the other hand, perhaps this version is also vulnerable to resource loss in which case I'm at a loss ^_^ )

main = do

sock <- socket AF_INET Stream 0

addr <- SockAddrInet 7777 <$> lookupHost "localhost"

setSocketOption sock ReuseAddr 1

bindSocket sock addr

listen sock 6

forkIO $ do

tid <- myThreadId

forkIO $ do

sleep 10

print "killing"

forkIO $ killThread tid >> print "killed it"

return ()

l <- bracketForkIO (opener sock) closer listener

sleep 2

killThread l

print "sleeping"

sleep 120

bracketForkIO :: IO a -> (a -> IO b) -> (a -> IO c) -> IO ThreadId

bracketForkIO aquire release k =

mask $ \restore -> do

x <- aquire

forkIO $ do

restore (k x) `onException` release x

release x

return ()

On Wed, Nov 12, 2014 at 6:28 PM, John Lato wrote:

On Thu Nov 13 2014 at 8:58:12 AM Yuras Shumovich wrote:

...

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:

People have been arguing the opposite. hClose is not guaranteed to close the handle in case an exception arises. Here's a demonstration program.

...

module Main where

import System.IO import Network.BSD import Network.Socket import Control.Concurrent import Control.Exception import Control.Applicative import Control.Monad

main = do sock <- socket AF_INET Stream 0 addr <- SockAddrInet 7777 <$> lookupHost "localhost" setSocketOption sock ReuseAddr 1 bindSocket sock addr listen sock 6 forkIO $ do tid <- myThreadId forkIO $ do sleep 10 print "killing" forkIO $ killThread tid >> print "killed it" return () bracket (opener sock) closer $ \h -> do forkIO $ listener h sleep 2 print "sleeping" sleep 120

listener h = forever $ do inp <- hGetLine h print inp

opener sock = do (s',addr) <- accept sock print $ "Got connection from: " ++ show addr socketToHandle s' ReadWriteMode

closer h = (hClose h `finally` print "closed")

sleep :: Double -> IO () sleep = threadDelay . round . (* 1e6)

lookupHost n = head . hostAddresses <$> getHostByName n

I compiled this with ghc-7.8.3 -O -threaded and ran it, then connected to localhost:7777 via nc, waited until "closed" was printed, then sent some data. This was the result:

jwlato@burial:~/explorations$ ./HClose "sleeping" "Got connection from: 127.0.0.1:45949" "killing" "killed it" "closed" "foo" "bar" "baz" HClose: : hGetLine: end of file

Note that "closed" was printed, so we should assume that hClose had a chance to run as well. The handle clearly was not closed (I confirmed this with lsof as well).

This result is consistent with the async exception arising while hClose is blocked internally on an MVar.

If you instead wrap "closer" in uninterruptibleMask_, the result is quite different:

jwlato@burial:~/explorations$ ./HClose "sleeping" "Got connection from: 127.0.0.1:46302" "killing" "foo" "closed" HClose: : hGetLine: illegal operation (handle is closed) "killed it

Note that this time, killThread didn't return immediately, because the async exception had not been delivered. As soon as I sent a line of data over the socket, hGetLine finished and unblocked hClose, which then closed the handle.. The next loop of hGetLine then failed, and concurrently killThread returned.

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.

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.

...

...

...

...

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

...

has bigger issue -- you may use already closed handle.) But it

It is already implemented in such the way. Let me explain. probably 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/805ee118b823f271dfd8036d35b15e b3454a95ad/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.

except that isn't guaranteed, as my program demonstrates.

...

...

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.

Sync exceptions have nothing to do with the proposal. The proposal itself certainly doesn't argue this.

...

...

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.

Again, this has nothing to do with hClose throwing sync exceptions. It does have to do with handlers that perform blocking operations and don't use uninterruptibleMask.

...

...

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

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

-- Eric Mertens

It's not vulnerable to the same issue because hClose can't block (as far as I know) because it can't be in use at the time that clean up is running. On Wed, Nov 12, 2014 at 7:40 PM, Merijn Verstraaten wrote:

...

On 12 Nov 2014, at 19:17, Eric Mertens wrote: First let's have a discussion about whether or not that program *should* have worked as written. It isn't immediately obvious that Handles should be closed out from under other threads that are using them like that. I certainly try to avoid it.

No, let's NOT discuss that. The documented behaviour for handles is that if closed in one thread, they become invalid in all threads. This also mimics the behaviour of FILE* in C and more importantly, while I agree that this design is not nice, solving it in a nice way is an open problem.

So let's not get sidetracked with a discussion of what Handle semantics should be in an ideal world. In the current real world if a programmer writes "hClose", let's assume he/she intends to close the Handle.

...

For example, we could modify the program like this so that the thread was responsible for cleaning up its own resources.

Your version is susceptible to the exact same problem we're discussing. If the "release" action blocks (as, for example, hClose potentially does) it can be interrupted and aborted.

Cheers, Merijn

-- Eric Mertens

On Wed, 2014-11-12 at 19:53 -0800, Merijn Verstraaten wrote:

...

On 12 Nov 2014, at 19:47, Eric Mertens wrote:

It's not vulnerable to the same issue because hClose can't block (as far as I know) because it can't be in use at the time that clean up is running.

Ah, you're right, but that's just because in this case it was possible to redesign the code not to expose the Handle. However, the fact that this happens to be possible in this small test program, doesn't guarantee all programs can be restructured this way (me and Eyal have both run into situations where we couldn't).

Sorry, I missed this email initially, so please ignore my question in the email I just sent.

Additionally, the only reason you know this is safe is because you've inspected (or assumed) the code dealing with handles and hClose and have checked that it isn't somehow sharing the Handle behind your back. Unless you propose every person always makes this analysis for any IO allocation operations they use (I hope not!), this remains an issue.

You continue referring to this issue, but I still don't understand it. Could you please elaborate it? There docs are clear: there is a list of uninterruptible actions, and you should assume all other actions interruptible. So that hClose -- no need to inspect anything. And hClose tries hard to close file descriptor in case of exception (sync or async). It is bug in hClose, not in bracket, if it fails to close the descriptor.

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On Wed, Nov 12, 2014 at 10:47 PM, Eric Mertens wrote:

It's not vulnerable to the same issue because hClose can't block (as far as I know) because it can't be in use at the time that clean up is running.

This sounds unsafe to me. System file descriptors are per process, and there are at least three such that *must* be per process (stdin, stdout, stderr). And, while closing those isn't especially common, it can be and is done sometimes. Can you guarantee that the corresponding handle's not in use? -- brandon s allbery kf8nh sine nomine associates allbery.b@gmail.com ballbery@sinenomine.net unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

On Wed, 2014-11-12 at 19:40 -0800, Merijn Verstraaten wrote:

...

On 12 Nov 2014, at 19:17, Eric Mertens wrote: First let's have a discussion about whether or not that program *should* have worked as written. It isn't immediately obvious that Handles should be closed out from under other threads that are using them like that. I certainly try to avoid it.

No, let's NOT discuss that. The documented behaviour for handles is that if closed in one thread, they become invalid in all threads. This also mimics the behaviour of FILE* in C and more importantly, while I agree that this design is not nice, solving it in a nice way is an open problem.

So let's not get sidetracked with a discussion of what Handle semantics should be in an ideal world. In the current real world if a programmer writes "hClose", let's assume he/she intends to close the Handle.

...

For example, we could modify the program like this so that the thread was responsible for cleaning up its own resources.

Your version is susceptible to the exact same problem we're discussing. If the "release" action blocks (as, for example, hClose potentially does) it can be interrupted and aborted.

How can it? takeMVar will not be interrupted because the mvar is full. hClose performs other interruptible actions (like flushing internal buffer,) but they seems to be correctly handled.

Cheers, Merijn

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On Thu, Nov 13, 2014 at 11:46 AM, Yuras Shumovich 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

On Thu, 2014-11-13 at 02:28 +0000, John Lato wrote: 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

On Thu, Nov 13, 2014 at 1:21 PM, Yuras Shumovich wrote:

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

That is true - and indeed my code does actually have a terminateProcess https://github.com/ElastiLotem/buildsome/blob/master/src/Lib/Process.hs#L21-... in there right before waiting for the process (and more stuff too). And even if the process decides never to terminate, it is reasonable for the thread that represents it to never terminate just as well.

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.

Which, like all cleanup handlers, consists first and foremost of wrapping it with uninterruptibleMask. Then, you can also try to ask whether it has any problematic sync exceptions you want to handle in some way - and do that too (independently).

...

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.

And you don't want to interrupt it because that would be worse than blocking -- it would just lose the side effects of closing that you're after.

The alternative is to wrap takeMVar into uninterruptibleMask, in that case hClose becomes exception safe and interruptible.

When you interrupt it, does it close or not? Does it flush the buffers or not?

...

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.

No, the code can be sync-exception-safe (as far as possible, anyway) but async exceptions will still wreck havoc. For example, your "hClose h1 `finally` hClose h2" code which is "sync-correct" but still async-incorrect.

...

...

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.

For cleanup handlers, correctness w.r.t async exceptions is to postpone them until after the cleanup is complete.

...

...

Thanks, Yuras

-- Eyal

On 07:16, Thu, Nov 13, 2014 Yuras Shumovich wrote: On Thu, 2014-11-13 at 15:51 +0200, Eyal Lotem wrote:

On Thu, Nov 13, 2014 at 1:21 PM, Yuras Shumovich wrote:

...

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

That is true - and indeed my code does actually have a terminateProcess < https://github.com/ElastiLotem/buildsome/blob/master/src/Lib/Process.hs#L21-...

in there right before waiting for the process (and more stuff too).

Hmm, at the first glance, the code looks overengineered. I wonder whether it is correct even under uninterruptibleMask. I may miss the point though, but I'm not surprised you had hard times with it. I think auditing code to see if it should be refactored to avoid this problem is fruitful. Functions like bracket should "just work", so they can be used reliably even by novice programmers. Currently that's not the case. Your objection seems to be that users should know how to write reliable handlers. While that's certainly true, I think we should make it as easy as possible to do so. Deep knowledge of async exceptions is IMHO just plain excessive.

I'm a little late to this discussion, but i just wanted to say that I'm +1 on potential solution #2 (Change bracket to use uninterruptible cleanup handler). I remember I was really surprised the first time I studied `bracket` precisely because of this weird interruptible cleanup behavior. Sensible defaults matter, and `bracket` by virtue of its simpler name is the implicitly suggested default. I'm also pretty sure that the amount of breakage will be minimal and easy to fix (the very few affected libraries can just use ifdefs and bracketInterruptible to work around this) and the amount of fixage will be maximal.

On 12:31, Thu, Nov 13, 2014 Yuras Shumovich wrote: TLDR: the proposal is still a bad idea :)))))) IMHO it's better than the current situation. On Thu, 2014-11-13 at 18:20 +0000, John Lato wrote:

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.

hClose is pervasive, but it doesn't mean the bug in it affects a lot of code. Probably 99.(9)% hClose uses are correct. (it may affect more code then I think, but nobody even created a ticket yet!) Do you really think it is common to close handle while using it from other thread? No, but that's not a general requirement for this problem to manifest. Besides, why shouldn't one be able to safely close a handle while another thread may be using it, without using arcane functions? But yes, there is a lot of broken code in libraries. But we should fix bugs instead if hiding them. This isn't about hiding bugs. It's about changing the semantics of exception handlers so that they are more likely to match what's desired in the general case.

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.

Haskellers don't *want* to worry about async exception, but then should. Otherwise let remove exception from language (yes, including synchronous -- average Haskeller doesn't worry about them too.) Why should Haskellers worry about async exceptions? The vast majority will never use them. They are difficult to reason about and difficult to use properly, and it's only because certain aspects of the RTS (and killThread) are implemented with them that most people ever encounter them. The proposal makes it easer to continue ignoring async exceptions. That is why I'm arguing here against it. (Possible breakage if existing code worries me too, but much less) I think it's a good thing to make it easier to ignore async exceptions. It is common myth that bracket saves you from async exceptions, but that is simply not true. And the proposal will not make it true. So newcomers learn to ignore async exceptions and continue doing that forever. Even worse, when newcomer finally find out (most likely itself!!!), that the myth is wrong, he doesn't get help from the community -- no docs, no tutorials, no blogs. Everything he can find is a set of myths. Or we could make the implementation match the common knowledge. Then it wouldn't be a myth, it would be true. I started learning haskell 8 years ago, and I'm paid for haskell code 3 years already. When do you think I discovered that bracket is not enough to handle async exceptions? Half a year ago(!) I'll post the link again: http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5... Note that nobody answered the question about hClose. Nobody explained how to use interruptible exceptions in cleanup. Very few people actually even care to replay. Because everybody learned to ignore async exceptions. Did you read this: http://www.well-typed.com/blog/97/ ? I hope you did. Because it is probably the *only* deep discussion of *some* of the related issues. Unfortunately it appears after too late for me, so I spent a lot of days discovering everything myself. Wouldn't it be better if stuff like that just worked out of the box? People could still spend days learning it, but they wouldn't be bitten by these hard-to-identify, poorly-understood bugs. I even wrote my own library for exception handling: https://github.com/Yuras/io-region/ I'm still not sure it is good. It even can be buggy. But I considered a lot of design decisions, including unintrruptibleMask, and found then unsatisfactory . I personally find it easer to write exception safe code with io-region, but you should understand async exceptions anyway. There is no easy way unfortunately. That doesn't mean we should make it harder to do the right thing. John

...

...

...

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?

...

...

...

...

So sync exceptions in hClose mean the program is incorrect, and

I'm referring to this: the

...

only

...

...

...

recourse is to prevent the sync exceptions in the first place.

What can it means except banning sync exceptions? Actually I seriously considered baning exceptions in cleanup action, but found it unsatisfactory Sorry for long email. Thanks, Yuras

On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich wrote:

...

On 12:31, Thu, Nov 13, 2014 Yuras Shumovich wrote:

TLDR: the proposal is still a bad idea :))))))

IMHO it's better than the current situation.

On Thu, 2014-11-13 at 18:20 +0000, John Lato wrote:

...

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

On Thu, 2014-11-13 at 21:06 +0000, John Lato wrote: likely

...

...

in libraries.

hClose is pervasive, but it doesn't mean the bug in it affects a lot of code. Probably 99.(9)% hClose uses are correct. (it may affect more code then I think, but nobody even created a ticket yet!) Do you really think it is common to close handle while using it from other thread?

No, but that's not a general requirement for this problem to manifest.

Sorry, I didn't get it. You mean concurrent access to handle is not a general requirement for the bug in hClose to manifest itself? There are other cases where hClose leaks file descriptor?

...

Besides, why shouldn't one be able to safely close a handle while another thread may be using it, without using arcane functions?

We *should* be able. So lets fix hClose.

...

But yes, there is a lot of broken code in libraries. But we should fix bugs instead if hiding them.

This isn't about hiding bugs. It's about changing the semantics of exception handlers so that they are more likely to match what's desired

in

...

the general case.

This *is* about hiding bugs. I believe that in most cases async exceptions uncover bugs that are not actually async-only. So using uninterruptibleMask you are hiding real bug. And I already gave an example, I'll reproduce it here:

data DB = DB Handle Handle

closeDB :: DB -> IO () closeDB (DB h1 h2) = hClose h1 >> hClose h2

Here closeDB is buggy with respect to both sync and async exceptions. Fixing it is trivial:

-- here I assume that the bug in hClose is fixed closeDB (DB h1 h2) = hClose h1 `finally` hClose h2

That fixes it w.r.t. sync *and* async exception without any special work for async case.

NO: This new code is still broken. Async exception in close of h1 will just "jump" to block at h2, and yield the full side effect of closing h2 while skipping some of the side effect of closing h1. All my code used these kind of idioms for sync-exception-safety but was still ridden with bugs w.r.t async exceptions. The kill&wait for process are another example, but I have multiple examples -- all of which become more reasonable when the cancellation is uninterruptible.

Almost nobody test code for case when file is deleted, so it is unlikely to discover the bug in the first version in case of uninterruptibleMask. So the proposal *hides* the bug, probably for 99% cases, but the bug is here, and it will bit you sooner or later.

Are you talking about Windows or POSIX? With POSIX, file deletion has nothing to do with hClose.

...

The proposal makes it easer to continue ignoring async exceptions. That is why I'm arguing here against it. (Possible breakage if existing code worries me too, but much less)

I think it's a good thing to make it easier to ignore async exceptions.

It is already easy -- just don't use them.

Whenever you use the async library, for example, you use async exceptions. And then all your bracket invariants are broken *by-default*.

Or wrap our main into uninterruptibleMask :)

main :: IO () main = uninterruptibleMask_ $ do ....

But cancelling/killing non-cleanups is not problematic in general.

...

It is common myth that bracket saves you from async exceptions, but that is simply not true. And the proposal will not make it true. So newcomers learn to ignore async exceptions and continue doing that forever.

Even worse, when newcomer finally find out (most likely itself!!!), that the myth is wrong, he doesn't get help from the community -- no docs, no tutorials, no blogs. Everything he can find is a set of myths.

Or we could make the implementation match the common knowledge. Then it wouldn't be a myth, it would be true.

No, it will *not* be true. Did you read the article I mentioned?

...

I started learning haskell 8 years ago, and I'm paid for haskell code 3 years already. When do you think I discovered that bracket is not enough to handle async exceptions? Half a year ago(!)

I'll post the link again:

http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5...

...

Note that nobody answered the question about hClose. Nobody explained how to use interruptible exceptions in cleanup. Very few people actually even care to replay. Because everybody learned to ignore async exceptions.

Did you read this: http://www.well-typed.com/blog/97/ ? I hope you did. Because it is probably the *only* deep discussion of *some* of the related issues. Unfortunately it appears after too late for me, so I spent a lot of days discovering everything myself.

Wouldn't it be better if stuff like that just worked out of the box? People could still spend days learning it, but they wouldn't be bitten by these hard-to-identify, poorly-understood bugs.

I even wrote my own library for exception handling: https://github.com/Yuras/io-region/ I'm still not sure it is good. It even can be buggy. But I considered a lot of design decisions, including unintrruptibleMask, and found then unsatisfactory

.

I personally find it easer to write exception safe code with io-region, but you should understand async exceptions anyway. There is no easy way unfortunately.

That doesn't mean we should make it harder to do the right thing.

The proposal doesn't make it significantly easer. It just hides bugs.

The current situation hides bugs -- virtually all uses of bracket in the wild leak resources and break invariants in their cancellation if async exceptions are thrown, whereas with the proposal they all become correct w.r.t async exceptions modulu deadlock concerns.

It is a myth that handling async exceptions in cleanup is much harder then handling sync exceptions. Async exceptions are hard to deal with because they can be raised at any point (even between points :) ). But in cleanup action async exceptions are masked, and can be raised only in well known points, just like regular sync exceptions.

Sync exceptions are under your control. You can make sure the preconditions of the operation are met so that they are not raised. If they are raised, they are related to the operation at hand so there may be something that you can do. Async exceptions are fundamentally different, so please stop mishmashing these two dissimilar things together. -- Eyal

On Fri, Nov 14, 2014 at 1:00 AM, Yuras Shumovich wrote:

On Fri, 2014-11-14 at 00:33 +0200, Eyal Lotem wrote:

...

On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich wrote:

...

That fixes it w.r.t. sync *and* async exception without any special work for async case.

NO: This new code is still broken. Async exception in close of h1 will just "jump" to block at h2, and yield the full side effect of closing h2 while skipping some of the side effect of closing h1.

Yes, but that is good default, I already explained why. If you care about about side effect in case of async exception, when use uninterruptibleMask explicitly, and explain in comments why you need that.

I disagree that it is a good default. "withFile" combined with successful writes to the handle now no longer guarantee the writes occur. Your writes get lost forever despite diligently using the correct idioms, whenever async exceptions are involved.

I argue that it is *very* rare that your cleanups have anything sensible to do upon async exceptions (I don't think I've ever encountered a single case where that was desirable) and yet it is very common for cleanups to "forget" to uninterruptible-mask when they should (almost always). Again, this comes up when you review random uses of bracket, which are broken in virtually every case of an interruptible cleanup. The sane default is to guarantee that the cleanup happens even if there is an async exception. Then someone who has some sensible thing to do when an async exception occurs during cleanup can go ahead and use the rarely-useful bracketInterruptible.

...

All my code used these kind of idioms for sync-exception-safety but was still ridden with bugs w.r.t async exceptions.

Several options: a) your code is buggy, fix it b) you rely on buggy code, pester it's author to fix it (and temporary use uninterruptibleMask) c) you really need uninterruptibleMask here, go ahead and use it

Why don't we consider some realistic examples? withFile => you want it to flush successful writes => You want uninterruptibleMask. withMVar => you want it to guarantee the MVar doesn't remain in an inconsistent state => You want uninterruptibleMask createProcess/kill+wait => you want to guarantee that the process-bracket actually maintains an invariant about the process's state => you want uninterruptibleMask withSemaphore => you want to guarantee that the semaphore doesn't remain in an inconsistent state => you want uninterruptibleMask See a pattern here?

...

The kill&wait for process are another example, but I have multiple

examples

...

-- all of which become more reasonable when the cancellation is uninterruptible.

Then just use uninterruptibleMask if it is reasonable in your case.

...

...

Almost nobody test code for case when file is deleted, so it is

unlikely

...

...

to discover the bug in the first version in case of uninterruptibleMask. So the proposal *hides* the bug, probably for 99% cases, but the bug is here, and it will bit you sooner or later.

Are you talking about Windows or POSIX? With POSIX, file deletion has nothing to do with hClose.

Ok, Almost nobody test code for case when <insert a case when your cleanup action throws sync exception>

sync exceptions are irrelevant here.

...

...

...

The proposal makes it easer to continue ignoring async exceptions.

That

...

...

...

is why I'm arguing here against it. (Possible breakage if existing code worries me too, but much less)

I think it's a good thing to make it easier to ignore async exceptions.

It is already easy -- just don't use them.

Whenever you use the async library, for example, you use async exceptions. And then all your bracket invariants are broken *by-default*.

Don't use async then.

Whereas with this proposal, people can safely use async and bracket will work correctly.

...

...

Or wrap our main into uninterruptibleMask :)

main :: IO () main = uninterruptibleMask_ $ do ....

But cancelling/killing non-cleanups is not problematic in general.

Did you read "Dealing with Asynchronous Exceptions during Resource Acquisition" article?

Yes. I didn't say it's not tricky -- I said it's not problematic. i.e: It's very possible to make resource acquisition cancelable - and it is often critical that we can abort threads that are still at resource acquisition stage.

...

...

It is a myth that handling async exceptions in cleanup is much harder then handling sync exceptions. Async exceptions are hard to deal with because they can be raised at any point (even between points :) ). But in cleanup action async exceptions are masked, and can be raised only

in

...

well known points, just like regular sync exceptions.

Sync exceptions are under your control. You can make sure the preconditions of the operation are met so that they are not raised. If they are raised, they are related to the operation at hand so there may be something that you can do. Async exceptions are fundamentally different, so please stop mishmashing these two dissimilar things together.

So you want to ban sync exceptions in cleanup actions?

What?? No. I want to write my code in such a way that eliminates as many sync exceptions as possible. For example, I will avoid passing invalid handles to hClose - and I have a guarantee that one sync exception will not happen. -- Eyal

On Fri, Nov 14, 2014 at 2:56 AM, Yuras Shumovich wrote:

On Fri, 2014-11-14 at 01:54 +0200, Eyal Lotem wrote:

...

On Fri, Nov 14, 2014 at 1:00 AM, Yuras Shumovich wrote:

...

On Fri, 2014-11-14 at 00:33 +0200, Eyal Lotem wrote:

...

On Fri, Nov 14, 2014 at 12:24 AM, Yuras Shumovich < shumovichy@gmail.com> wrote:

...

That fixes it w.r.t. sync *and* async exception without any special work for async case.

NO: This new code is still broken. Async exception in close of h1 will just "jump" to block at h2, and yield the full side effect of closing h2 while skipping some of the side effect of closing h1.

Yes, but that is good default, I already explained why. If you care about about side effect in case of async exception, when use uninterruptibleMask explicitly, and explain in comments why you need that.

I disagree that it is a good default. "withFile" combined with successful writes to the handle now no longer guarantee the writes occur. Your writes get lost forever despite diligently using the correct idioms, whenever async exceptions are involved.

You are missing the point. Async exception can interrupt "withFile" in acquire, cleanup or body, and you can't rely on any particular case. So there should not be any difference whether write to the handle was lost because of hPutStr was interrupted, or because hClose failed to flush buffers. withFile doesn't provide atomic guaranties.

You misunderstand me. With your proposal for hClose and current bracket, users would no longer have any useful guarantee about *success* results from writes to handles *inside* the withFile bracket. But I think the entire discussion focused too much on hClose whereas other examples make bracket's terrible default much clearer.

But you can propose to change the default behavior of hClose if you find it problematic. It is not relevant here.

...

I argue that it is *very* rare that your cleanups have anything sensible

to

...

do upon async exceptions (I don't think I've ever encountered a single case where that was desirable) and yet it is very common for cleanups to "forget" to uninterruptible-mask when they should (almost always). Again, this comes up when you review random uses of bracket, which are broken in virtually every case of an interruptible cleanup.

The sane default is to guarantee that the cleanup happens even if there is an async exception. Then someone who has some sensible thing to do when an async exception occurs during cleanup can go ahead and use the rarely-useful bracketInterruptible.

...

...

All my code used these kind of idioms for sync-exception-safety but

was

...

still ridden with bugs w.r.t async exceptions.

Several options: a) your code is buggy, fix it b) you rely on buggy code, pester it's author to fix it (and temporary use uninterruptibleMask) c) you really need uninterruptibleMask here, go ahead and use it

Why don't we consider some realistic examples?

withFile => you want it to flush successful writes => You want uninterruptibleMask.

No, I don't, see above.

...

withMVar => you want it to guarantee the MVar doesn't remain in an inconsistent state => You want uninterruptibleMask

I don't want it to block indefinitely if I incidentally put something into the mvar somewhere else.

You want to go to an incoherent program state, instead?!

If you don't manipulate the mvar yourself (and you shouldn't actually), then withMVar does not leave the mvar in inconsistent state. I don't want uninteruptibleMask here.

It is entirely plausible to have a program where the MVar contains some resource, and some background thread fills that mvar from some resource pool and eventually restores a resource to the pool (bracket putMVar takeMVar) - and another thread pulls the resource from the mvar to use it and eventually puts the resource back into the mvar. i.e: you have concurrent reversed brackets: bracket takeMVar putMVar bracket putMVar takeMVar NOTE: Only in such situations the cleanup here may be interruptible at all. Of course any interruptible mvar operation will *not* occur if it is interrupted. Of course we want our cleanup to not break our mvar's invariants. So of course any cleanup involving interruptible mvar operations will want these operations *not to be* interruptible during cleanup.

...

createProcess/kill+wait => you want to guarantee that the process-bracket actually maintains an invariant about the process's state => you want uninterruptibleMask

I don't want it to block for unbound amount of time. I don't want uninteruptibleMask here.

You prefer it to break program invariants? I don't understand how you could possible prefer random breakage over long blocking. Not to mention: If you don't care that the process is actually terminated in an event of an exception -- why would you use bracket at all? If I put the "terminateProcess `finally` waitForProcess" in my bracket cleanup -- it doesn't say: "Please clean up this process on a good day". It is saying: "My program must not have this process running by the time this bracket completes". I think if you disagree on this -- then we simply have a completely different understanding of what "bracket" is meant to achieve in the first place!

...

withSemaphore => you want to guarantee that the semaphore doesn't remain in an inconsistent state => you want uninterruptibleMask

I don't remember anything about it, but I'm not sure I'd want uninterruptibleMask here.

Again, if you prefer to prevent unbounded blocking at the expense of randomly breaking program invariants -- of course you don't want it here. But if you agree that upholding program invariants is a basic prerequisite for program correctness, then you cannot possibly hold this position.

...

See a pattern here?

Yes, but it probably differs from your's :)

...

...

...

The kill&wait for process are another example, but I have multiple

examples

...

-- all of which become more reasonable when the cancellation is uninterruptible.

Then just use uninterruptibleMask if it is reasonable in your case.

...

...

Almost nobody test code for case when file is deleted, so it is

unlikely

...

...

to discover the bug in the first version in case of uninterruptibleMask. So the proposal *hides* the bug, probably for 99% cases, but the

bug is

...

...

...

...

here, and it will bit you sooner or later.

Are you talking about Windows or POSIX? With POSIX, file deletion has nothing to do with hClose.

Ok, Almost nobody test code for case when <insert a case when your cleanup action throws sync exception>

sync exceptions are irrelevant here.

Ok. Sync exceptions are irrelevant here. For you.

...

...

...

...

...

The proposal makes it easer to continue ignoring async

exceptions.

...

...

That

...

...

...

is why I'm arguing here against it. (Possible breakage if existing code worries me too, but much less)

I think it's a good thing to make it easier to ignore async exceptions.

It is already easy -- just don't use them.

Whenever you use the async library, for example, you use async exceptions. And then all your bracket invariants are broken *by-default*.

Don't use async then.

Whereas with this proposal, people can safely use async and bracket will work correctly.

...

...

...

Or wrap our main into uninterruptibleMask :)

main :: IO () main = uninterruptibleMask_ $ do ....

But cancelling/killing non-cleanups is not problematic in general.

Did you read "Dealing with Asynchronous Exceptions during Resource Acquisition" article?

Yes. I didn't say it's not tricky -- I said it's not problematic. i.e: It's very possible to make resource acquisition cancelable - and it is often critical that we can abort threads that are still at resource acquisition stage.

You didn't recognized symmetry between acquire and cleanup? I even can replicate similar bug as in hClose, but in acquire. I can't understand why you find cleanup problematic, but acquire -- not.

Because exceptions during an IO action are supposed to: A) prevent the side-effects of that IO action from occuring, when possible A.2) avoid leaking any partially-allocated resources These general IO action requirements make it safe to abort a bracket allocation, as if the bracket was never entered in the first place. However, for a cleanup -- these do not help! In a cleanup, "abort" is virtually always not an option, unless again you are willing to throw away your program invariants. This is why the two are not symmetric, and letting the allocation be interrupted as long as it is a well-founded IO action is fine, whereas for cleanup an abort is simply going to wreak havoc in almost every case.

...

...

...

...

It is a myth that handling async exceptions in cleanup is much

...

...

...

...

then handling sync exceptions. Async exceptions are hard to deal with because they can be raised at any point (even between points :) ). But in cleanup action async exceptions are masked, and can be raised only in well known points, just like regular sync exceptions.

Sync exceptions are under your control. You can make sure the preconditions of the operation are met so that they are not raised. If they are raised, they are related to the operation at hand so there may be something

harder that

...

...

...

you can do. Async exceptions are fundamentally different, so please stop mishmashing these two dissimilar things together.

So you want to ban sync exceptions in cleanup actions?

What?? No.

I want to write my code in such a way that eliminates as many sync exceptions as possible. For example, I will avoid passing invalid handles to hClose - and I have a guarantee that one sync exception will not happen.

Ok, but what about exceptions that you can't eliminate? You either ban them or handle them. If you handle them, then goto "It is a myth that handling async exceptions in cleanup is much harder then handling sync exceptions"

There is no real way to "handle" most sync exceptions in cleanups. For example, your "handling" of exceptions in the double-close example does not "handle" the exception at all, it just contains the damage done (failure to close the first handle) to the first handle, but the damage was done. If a cleanup fails with a sync exception, either you retry, or you failed to clean up and you've leaked a resource or broke an invariant. In some cases you could try to escalate the cleanup to a higher level where it is still possible, but that requires system-wide design around this and cannot be done in a simple exception handler. Thus, sync exceptions are in a sense a lost cause when it comes to robustly handling them, at least locally. Just do your best to prevent them from happening -- and indeed contain their damage as much as possible (as you did by using `finally` to continue the cleanups you still can salvage). Async exceptions, however, are possible to robustly prevent. If you let them interrupt your cleanup and then try to handle them -- you've already lost the cleanup work that was supposed to happen, and the game is over, it is too late. -- Eyal

On 12 Nov 2014, at 16:55, Yuras Shumovich wrote:

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.

This proposal does not say anything about handling synchronous exceptions and anyone who said it does is mistaken. mask and uninterruptibleMask do not affect synchronous exceptions, so we can ignore synchronous exceptions for this discussion. Cheers, Merijn

On 13/11/2014 10:58, Simon Marlow wrote:

On 11/11/2014 23:28, Yuras Shumovich wrote:

...

On Wed, 2014-11-12 at 02:02 +0300, Yuras Shumovich wrote:

...

On Tue, 2014-11-11 at 22:14 +0000, John Lato wrote:

...

I don't follow your argument. You state that a user should assume their cleanup action can block.

Lets use accurate terms. You probably mean "can be interrupted". If action can block, then it is may (usual case) or may not (e.g. when async exceptions are masked) be interruptible. Please let me know if you disagree with this definitions -- I just want to ensure everybody uses the same definitions.

...

Blocking implies that the function may receive an async exception. The only way to guarantee code runs in the presence of async exceptions is uninterruptibleMask (or something built on it).

I don't understand that. The code definitely runs with or without uninterruptibleMask. In the first case it can be interrupted by async exception. But in both cases it can throw sync exception.

Hmm, I reread your email, and now I probably see what you mean. You think about cleanup as atomic action that either runs or is interrupted. But that is not true. Consider the example I already used earlier:

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

But in order to know that this is correct, you have to know how hClose works, namely that if an async exception is raised then it has safely closed the handle anyway. If you didn't know that, then you would have to code it like this instead

closeDB (DB h1 h2) = cleanup1 where cleanup1 = (hClose h1 `onException` cleanup1) >> cleanup2 cleanup2 = hClose h2 `onException` cleanup2

Does this really work? I think so, but I haven't tested it...

And having sent this, I now realise that of course it doesn't work in the case when hClose throws a synchronous exception every time. So this line of thought ends up at the conclusion that in this case we need uninterruptibleMask *and* hClose h1 `finally` hClose h2. Cheers, Simon

I think your point about async exceptions under mask just being a special case of sync exceptions is a good one, though. I'm now wondering whether maybe the right solution is to make it easier to use the idiom in the above code.

Cheers, Simon

...

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.

...

...

So doesn't that assumption imply that uninterruptibleMask should be the default?

No, I don't see how it implies that.

...

Although you also seem to be saying that functions like hClose etc. should use uninterruptibleMask internally anyway. Possibly, but I'm less convinced that's a good idea. It's a pretty blunt hammer to have hidden from the user, and if it has to be used bracket seems like a better choice because it will fix many cases instead of just one and library authors won't have to struggle to get it right.

No, I propose to write exception safe code in the first place. And use uninterruptibleMask if it is necessary (usually it is not necessary.)

Just adding uninterruptibleMask to bracket doesn't make is easer to write exception safe code. hClose can throw (synchronous) exceptions anyway, so you should be prepared. There is no other way except ensuring that hClose is exception-safe *and* you are using it in exception-safe manner.

...

If you can write an hClose implementation that does the right thing, is async-safe, and doesn't use uninterruptibleMask or the equivalent, I might be convinced the proposed solution is bad.

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

Basically it catches all exceptions (including async,) closes the handle and rethrows the exception.

Let me state it again: hClose closes the handle in case of exception, including async exception. And that is the only correct behavior -- every cleanup action should do that, otherwise it is not exception safe. Masking async exception doesn't magically make code exception safe.

...

John L.

On 13:03, Tue, Nov 11, 2014 Yuras Shumovich wrote:

...

On Tue, 2014-11-11 at 12:17 -0800, Merijn Verstraaten wrote:

...

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

It definitely can be done for cleanups too. E.g. using uninterruptibleMask as the last resort.

And "half a cleanup" is valid thing. E.g. database that uses two files:

data DB = DB Handle Handle

The following cleanup action is buggy:

close :: DB -> IO () close (DB h1 h2) = hClose h1 >> hClose h2

Note: it is broken regardless async exceptions(!) You can get half cleanup even under uninterruptibleMask

The code is either exception-safe or buggy. You can't magically fix buddy code using uninterruptibleMask.

...

I disagree that it should be left to the author of allocation operation

to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

(You probably mean "the author of *cleanup* operation"? I'll assume that)

Hm... You have to inspect code if you expect it to be buggy. Otherwise you should assume it is interruptible (unless listed in Cotrol.Exception module in "Interruptible operations" section or explicitly stated in other way), but all cleanup actions do full cleanup even if they throw exception. If the code is buggy -- it should be fixed, not hidden.

...

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.

You need to audit half of ecosystem anyway to ensure allocating actions are not buggy. And as the example above shows, even uninterruptibleMask doesn't guaranty anything.

I agree that current situation is bad. I have two drafts in my mailbox where I propose to use uninterruptibleMask in bracket, but I didn't send them -- every time I found that it doesn't fix anything actually. I don't know better solution, but the proposal is not even a solution.

Thanks, Yuras

...

Cheers, Merijn

> On 11 Nov 2014, at 11:58, Yuras Shumovich

...

> > Hello, > > Should we use `uninterrubtibleMask` for allocating action too? > > > I'm not sure my voice will be counted, but anyway, > I'm strong -1 because it fixes wrong issue. > > `hClose` is interruptible, but it closes the handle in any case. I'm > pretty sure. I ask that question (see > http://haskell.1045720.n5.nabble.com/Control-Exception- bracket-is-broken-td5752251.html ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly. > > I argue that cleanup action can be interruptible, but should ensure > cleanup is done. As the last resort, it should use `uninterrubtibleMask` > internally. > > Other issue is that a lot of allocating action are broken because > they > perform interruptible actions after allocating resource without handling > async exceptions. So my point is that masking async exceptions > solves > only one half of the issue while masking the other. > > Handling async exceptions is hard, and we can't make is easy using > `uninterrubtibleMask`. Instead we should educate ourselves to do it > correctly from the very beginning. There is only one alternative -- > remove async exceptions from haskell. > > To summarize, > - allocating action should either allocate resource or throw > exception; > it is a bug to allocate resource *and* throw exception > - cleanup action should release resource even if it throws an > exception > Developer should ensure both properties holds. > > Sorry my poor English. > > Thanks, > Yuras > > On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote: >> Ola! >> >> In September Eyal Lotem raised the issue of bracket's cleanup >> handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch. >> >> The problem, summarised: >> Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources. >> >> Main objection to making the cleanup handler uninterruptible: >> Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread. >> >> I count only two objections in the previous thread, 1 on the >> grounds

...

>> >> The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution. >> >> My counter to these objections is: >> 1) No one has yet shown me any code that relies on the cleanup handler being interruptible >> >> 2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally,

...

>> >> 3) Even GHC-HQ can't use bracket correctly (see Simon's emails) >> >> Potential solution #1: >> Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler. >> >> Potential solution #2: >> Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups. >> >> Trade-offs: >> Solution 1 won't change the semantics of any existing code, however

...

>> >> Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating

...

>> >> I will argue that solution #2 is preferable, since I have yet to >> see any code that uses the interruptibility of the cleanup handler. Whereas

...

>> >> Secondly, it is easier to detect deadlocks caused by this problem

wrote: that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem". potentially leaking file descriptors/handles. this also means that any currently broken uses of bracket will remain broken, possibly indefinitely. potential deadlocks in code that relies on the interruptibility of cleanup. there's many broken assumption assuming the cleanup handler is not interruptible. than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

...

>> >> [1] - https://www.haskell.org/pipermail/libraries/2014- September/023675.html >> >> Cheers, >> Merijn >> _______________________________________________ >> Libraries mailing list >> Libraries@haskell.org >> http://www.haskell.org/mailman/listinfo/libraries > >

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

I think the bigger objection to using uninterruptibleMask for the allocation phase of bracket is that it breaks this: withMVar m io = bracket (takeMVar m) (putMVar m) io Now withMVar will be uninterruptible while it is blocked, which will make a lot of common idioms unresponsive to async exceptions. This was the whole motivation behind the idea of interruptible operations. Cheers, Simon On 11/11/2014 20:17, Merijn Verstraaten wrote:

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

I disagree that it should be left to the author of allocation operation to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

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.

Cheers, Merijn

...

On 11 Nov 2014, at 11:58, Yuras Shumovich wrote:

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception-bracket-is-broken-td5... ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use `uninterrubtibleMask` internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On Thu, Nov 13, 2014 at 10:48 AM, Simon Marlow wrote:

I think the bigger objection to using uninterruptibleMask for the allocation phase of bracket is that it breaks this:

withMVar m io = bracket (takeMVar m) (putMVar m) io

Now withMVar will be uninterruptible while it is blocked, which will make a lot of common idioms unresponsive to async exceptions. This was the whole motivation behind the idea of interruptible operations.

But do you really prefer the alternative to this unresponsiveness? The alternative here is responsiveness, but it surely breaking the invariants of this MVar (the number of takes/puts will no longer match). I think it is far more important to maintain these kinds of invariants than to be responsive to async exceptions at any price. Also, if this is not responsive -- it means that someone is holding up the MVar, and responsiveness can still be attained by making sure that the holder stops hogging the mvar. That way you can maintain your invariants and be responsive, and this is the kind of approach I've used in my programs after using uninterruptibleMask for cleanups.

Cheers, Simon

On 11/11/2014 20:17, Merijn Verstraaten wrote:

...

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

I disagree that it should be left to the author of allocation operation to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

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.

Cheers, Merijn

On 11 Nov 2014, at 11:58, Yuras Shumovich wrote:

...

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.nabble.com/Control-Exception- bracket-is-broken-td5752251.html ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use `uninterrubtibleMask` internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014- September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

-- Eyal

On Thu, Nov 13, 2014 at 1:11 PM, Simon Marlow wrote:

On 13/11/2014 10:22, Eyal Lotem wrote:

...

On Thu, Nov 13, 2014 at 10:48 AM, Simon Marlow mailto:marlowsd@gmail.com> wrote:

I think the bigger objection to using uninterruptibleMask for the allocation phase of bracket is that it breaks this:

withMVar m io = bracket (takeMVar m) (putMVar m) io

Now withMVar will be uninterruptible while it is blocked, which will make a lot of common idioms unresponsive to async exceptions. This was the whole motivation behind the idea of interruptible operations.

But do you really prefer the alternative to this unresponsiveness?

To clarify I was responding here to the suggestion that the "allocation phase" (the first argument to bracket) should use uninterruptibleMask. The email I replied to quoted below begins "Allocation should not use uninterruptibleMask ...".

Oh, I see. I think there's a consensus that allocation cancellation is a very useful thing and must remain supported. i.e: the uninterruptibleMask only applies to the cleanup.

Cheers, Simon

The alternative here is responsiveness, but it surely breaking the

...

invariants of this MVar (the number of takes/puts will no longer match).

I think it is far more important to maintain these kinds of invariants than to be responsive to async exceptions at any price.

Also, if this is not responsive -- it means that someone is holding up the MVar, and responsiveness can still be attained by making sure that the holder stops hogging the mvar. That way you can maintain your invariants and be responsive, and this is the kind of approach I've used in my programs after using uninterruptibleMask for cleanups.

Cheers, Simon

On 11/11/2014 20:17, Merijn Verstraaten wrote:

Allocation should not use uninterruptibleMask as it is possible to handle async exceptions during allocation by nesting bracketOnError

Example: someFun mvar1 mvar2 = do (val1, val2) <- bracketOnError (takeMVar mvar1) (putMVar mvar1) (\x -> takeMVar mvar2 >>= \y -> return (x, y)))

This can be made nicer using the Cont monad to hide the marching to the left. The same cannot be done for cleanup, as there's no sane thing as "half a cleanup".

I disagree that it should be left to the author of allocation operation to ensure uninterruptibility as it is impossible to know whether a given IO blocks internally and thus should be masked without inspecting the *entire* code path potentially called by the cleanup handler.

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.

Cheers, Merijn

On 11 Nov 2014, at 11:58, Yuras Shumovich mailto:shumovichy@gmail.com> wrote:

Hello,

Should we use `uninterrubtibleMask` for allocating action too?

I'm not sure my voice will be counted, but anyway, I'm strong -1 because it fixes wrong issue.

`hClose` is interruptible, but it closes the handle in any case. I'm pretty sure. I ask that question (see http://haskell.1045720.n5.__nabble.com/Control-Exception-__ bracket-is-broken-td5752251.__html

<http://haskell.1045720.n5.nabble.com/Control-Exception- bracket-is-broken-td5752251.html> ) but didn't get any answer, so I read code and made experiments. IIRC `hClose` wraps internal interruptible action into `try` and handles everything correctly.

I argue that cleanup action can be interruptible, but should ensure cleanup is done. As the last resort, it should use `uninterrubtibleMask` internally.

Other issue is that a lot of allocating action are broken because they perform interruptible actions after allocating resource without handling async exceptions. So my point is that masking async exceptions solves only one half of the issue while masking the other.

Handling async exceptions is hard, and we can't make is easy using `uninterrubtibleMask`. Instead we should educate ourselves to do it correctly from the very beginning. There is only one alternative -- remove async exceptions from haskell.

To summarize, - allocating action should either allocate resource or throw exception; it is a bug to allocate resource *and* throw exception - cleanup action should release resource even if it throws an exception Developer should ensure both properties holds.

Sorry my poor English.

Thanks, Yuras

On Tue, 2014-11-11 at 10:09 -0800, Merijn Verstraaten wrote:

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/__pipermail/libraries/2014-__ September/023675.html <https://www.haskell.org/pipermail/libraries/2014- September/023675.html>

Cheers, Merijn _________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

_________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

_________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

-- Eyal

-- Eyal

Wouldn't it be nice if Haskell had a type system that could express concepts like uninterruptibility? In the mean time, I withdraw my endorsement of the proposal *solely* on the basis that I realize I don't understand the issues well enough.

On Wed, Nov 12, 2014 at 6:37 PM, John Lato wrote:

Do you think the current resource leaking issue is not particularly significant, or do you tend to think it's an issue that should be fixed, but you're weighting the unknown costs much more than I am?

I'm just change averse :) We have traditionally been very cavalier in the Haskell community about gratuitously changing APIs in ways that break user code, and in my opinion this is very bad and creates a perception for users that choosing Haskell is just going to end up creating work for them. In this regard I think it's very good to be conservative -- this function has been documented to behave in a certain way and you can be sure that somebody, somewhere relies on that behavior. Normally I argue strongly for this perspective (because it feels like few people do) on less serious things like changing function types or names, because those things break builds and cause people to scramble to restore their libraries to compatibility all at once after the GHC release. The consequences of *this* particular change, however, have the potential to be a lot worse than just making busywork for people: programs will silently continue to compile and then start deadlocking in mysterious (and likely rare/"race-condition-y") ways. G -- Gregory Collins

On 2014-11-13 04:25, Gregory Collins wrote:

On Wed, Nov 12, 2014 at 6:37 PM, John Lato wrote:

...

Do you think the current resource leaking issue is not particularly significant, or do you tend to think it's an issue that should be fixed, but you're weighting the unknown costs much more than I am?

I'm just change averse :)

We have traditionally been very cavalier in the Haskell community about gratuitously changing APIs in ways that break user code, and in my opinion this is very bad and creates a perception for users that choosing Haskell is just going to end up creating work for them. In this regard I think it's very good to be conservative -- this function has been documented to behave in a certain way and you can be sure that somebody, somewhere relies on that behavior.

I can certainly understand this perspective and *ordinarily* I'd agree with the stance that compatibility shouldn't be broken willy-nilly. That said, popularity isn't everything. Sometimes correctness *must* trump the convenience of not having to change client code. And clearly people haven't been leaving Haskell in droves on these grounds, so....

Normally I argue strongly for this perspective (because it feels like few people do) on less serious things like changing function types or names, because those things break builds and cause people to scramble to restore their libraries to compatibility all at once after the GHC release. The consequences of *this* particular change, however, have the potential to be a lot worse than just making busywork for people: programs will silently continue to compile and then start deadlocking in mysterious (and likely rare/"race-condition-y") ways.

... but such code is already broken in a *worse* way, namely in a hard-to-discover and hard-to-debug way. I'll take a nearly-instantly reproducible deadlock over a "reproducible-only-once-in-a-blue-moon" resource leak bug any day. This is what convinced me (and others) the last time around. What's changed since the last time this was discussed? Regards,

On Wed, Nov 12, 2014 at 10:59 PM, John Lato wrote:

Should we update the documentation of hClose to warn that it might not work?

hClose should be fixed to not leak the file descriptor. :) G -- Gregory Collins

On Nov 13, 2014 9:54 AM, "John Lato" wrote:

How about this: when is it correct for a cleanup handler to be

interrupted by an async exception and drop its cleanup action into the void? I am not sure that is ever correct, although I can see that it might be useful on rare occasions. Example: you issue an RPC upon receipt of an exception (perhaps to some centralized exception logging service). That's safe to try if running under mask (maybe you're OK with the thread being killed and canceling the RPC), but not uninterruptible mask.

Merijn pointed out that either a cleanup is not interruptible, in which case uninterruptibleMask doesn't change the semantics, or it is interruptible, in which case that's almost certainly a bug.

Data please :-). The fact is that currently it is possible to use bracket for control flows that will be unsafe after the proposed change. Maybe nobody is actually relying on that, but until someone does an audit I don't think we should accept hand waving assertions like this one way or the other. G

On Thu, Nov 13, 2014 at 10:59 AM, Bardur Arantsson wrote:

This sounds like a castle built on sand. One would hope that it's common knowledge to *not* attempt to do anything too ambitious in exception handlers -- and that even applies in languages *without* async exceptions! You log it or perhaps ship an event description to a different thread which can report the event, but you should definitely not be attempting network traffic in an exception handler.

Is this something you've actually seen in the wild or...?

Yes, for other languages. Unfortunately I can't give you more detail than that. And what, exactly, is it about exception handlers in Haskell that makes doing network calls from them a bad idea? I'm seeing a lot of assertions in this thread that seem rooted in opinion rather than facts. "People shouldn't do that!", you're arguing, before checking whether or not people actually do rely on being able to do it.

...

Merijn pointed out that either a cleanup is not interruptible, in which case uninterruptibleMask doesn't change the semantics, or it is interruptible, in which case that's almost certainly a bug.

Data please :-). The fact is that currently it is possible to use bracket for control flows that will be unsafe after the proposed change.

"Possible" does not mean "likely", so I'm going to ask for *your* data :).

The burden of proof has to be on those who propose to change the existing documented semantics. Am I being crazy or overcautious to want to insist on that? I don't think so. As others have already stated, the work required to do an audit of all

code using "bracket" is disproportionate to the risk of this change. (Or at least the risk that is perceived by the proponents of the change.)

Then we disagree on how potentially risky this change is. I've heard assertions like this before made many, many times right before some harmless-looking change causes massive breakage. I'll hope you understand if I'm not immediately convinced :). I'm not arguing the contra side here just to be obstinate. Rather, my experience tells me that the change is super scary (any similar change in how a core error-handling primitive behaves in the presence of concurrency would be), and that changes to routines like this one need to be done extremely carefully. If this change does break code, it is likely to do it in a way that is difficult to isolate and debug. G -- Gregory Collins

On 10:37, Thu, Nov 13, 2014 Gregory Collins wrote: On Nov 13, 2014 9:54 AM, "John Lato" wrote:

How about this: when is it correct for a cleanup handler to be

interrupted by an async exception and drop its cleanup action into the void? I am not sure that is ever correct, although I can see that it might be useful on rare occasions. Example: you issue an RPC upon receipt of an exception (perhaps to some centralized exception logging service). That's safe to try if running under mask (maybe you're OK with the thread being killed and canceling the RPC), but not uninterruptible mask.

Merijn pointed out that either a cleanup is not interruptible, in which case uninterruptibleMask doesn't change the semantics, or it is interruptible, in which case that's almost certainly a bug.

Data please :-). The fact is that currently it is possible to use bracket for control flows that will be unsafe after the proposed change. Maybe nobody is actually relying on that, but until someone does an audit I don't think we should accept hand waving assertions like this one way or the other. I think Bardur is right: that sort of thing is a bad idea in general explicitly because it can lead to deadlock. It might be possible now, but I think it should be explicitly disallowed, at least in the common case. It's possible I have little sympathy for breaking other people's code because mine has been broken quite frequently, but I still think the proposal is a step forward.

I'd like to emphasize that "bracket" is only one of multiple functions that needs to be changed. Some of those that need to be changed are primops. On Thu, Nov 13, 2014 at 9:47 AM, Merijn Verstraaten wrote:

...

On 12 Nov 2014, at 23:33, Bardur Arantsson wrote:

Is the code for this available for inspection somewhere? (I realize it might change based on the outcome of this "poll", but...)

There's no complete patch, but I believe the outline has been sketched before, anyway to repeat it. The current implementation of bracket is as follows:

bracket :: IO a -> (a -> IO b) -> (a -> IO c) -> IO c bracket before after thing = mask $ \restore -> do a <- before r <- restore (thing a) `onException` after a _ <- after a return r

A new version would look like:

bracket before after thing = mask $ \restore -> do let atomicAfter = uninterruptibleMask . after a <- before r <- restore (thing a) `onException` atomicAfter a _ <- atomicAfter a return r

Slightly different versions are possible and the other relevant bracketing functions mentioned in this thread can be treated similarly.

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

-- Eyal

On 13/11/2014 10:44, Simon Marlow wrote:

On 13/11/2014 07:47, Merijn Verstraaten wrote:

...

A new version would look like:

bracket before after thing = mask $ \restore -> do let atomicAfter = uninterruptibleMask . after a <- before r <- restore (thing a) `onException` atomicAfter a _ <- atomicAfter a return r

Slightly different versions are possible and the other relevant bracketing functions mentioned in this thread can be treated similarly.

Since we would need this for catch too, the sensible thing to do (if we decide to go ahead with this) would be to change the implementation of catch in the RTS from masking the exception handler to uninterruptibleMask. That would mean that at least for catch there would be no additional overhead, and it would make the modifications to the other operations simpler in some cases.

If this isn't done in the RTS, is there a possibility of an async exception slipping in between the exception handler starting and the uninterruptibleMask starting? Cheers, Ganesh

On 19/11/2014 23:07, Ganesh Sittampalam wrote:

On 13/11/2014 10:44, Simon Marlow wrote:

...

On 13/11/2014 07:47, Merijn Verstraaten wrote:

...

...

A new version would look like:

bracket before after thing = mask $ \restore -> do let atomicAfter = uninterruptibleMask . after a <- before r <- restore (thing a) `onException` atomicAfter a _ <- atomicAfter a return r

Slightly different versions are possible and the other relevant bracketing functions mentioned in this thread can be treated similarly.

Since we would need this for catch too, the sensible thing to do (if we decide to go ahead with this) would be to change the implementation of catch in the RTS from masking the exception handler to uninterruptibleMask. That would mean that at least for catch there would be no additional overhead, and it would make the modifications to the other operations simpler in some cases.

If this isn't done in the RTS, is there a possibility of an async exception slipping in between the exception handler starting and the uninterruptibleMask starting?

No, because the exception handler is masked. Cheers, Simon

On 2014-11-21 18:22, Gregory Collins wrote:

New post from Yuras provides food for thought: https://github.com/Yuras/io-region/wiki/Handling-%28async%29-exceptions-in-h...

In particular he points out a case for which uninterruptibleMask will cause unkillable threads: let's say hClose blocks flushing the output to a file, but the write fails because of a hardware error and blocks forever.

There's no such thing as a hardware error that blocks forever. At least not on any sane kernel.

(Alternatively, imagine the file is on NFS and you get a cable cut between the two machines). The thread executing hClose in the cleanup action becomes unkillable.

This is quite standard for NFS on Linux. Any process accessing a resource on NFS automatically becomes unkillable on Linux if you cut the cable -- that's because it gets stuck in the system call. (Theoretically it should be possible to specify timeouts and such, but I've never been able to get timeouts on these things working properly/reliably. The process just gets stuck forever regardless of what you do. Ctrl-C doesn't work. "Kill -9" doesn't work. Etc. YMMV.) There's nothing any application can do about this regardless of "interruptible/uninterruptible". NFS is fundamentally broken on Linux and probably on all other platforms too. Anyway: Could we not just agree to see what happens when this change goes live? People are used to a X.0 release having potential problems and conservative souls can always stay on (X-1).Y until potential kinks in the X series are worked out. (Which I would assume they would do anyway.) Hopefully, there'll also at least a couple of release candidates where this can be tested in practice. It's not like this is hard to roll the change back if it should prove problematic. Regards,

On Sun Nov 23 2014 at 7:09:10 AM Simon Marlow wrote:

On 21/11/14 17:22, Gregory Collins wrote:

...

New post from Yuras provides food for thought: https://github.com/Yuras/io-region/wiki/Handling-%28async% 29-exceptions-in-haskell:-pushing-bracket-to-the-limits

In particular he points out a case for which uninterruptibleMask will cause unkillable threads: let's say hClose blocks flushing the output to a file, but the write fails because of a hardware error and blocks forever. (Alternatively, imagine the file is on NFS and you get a cable cut between the two machines). The thread executing hClose in the cleanup action becomes unkillable.

Yes, and furthermore hClose is not "buggy": even if it is interrupted by an async exception, the file descriptor will still be closed by the finalizer. This is not something you want to do a lot, of course, but as a backup plan for the rare case of an async exception killing the cleanup action it's fine.

This is not entirely correct. If another thread is holding the MVar when hClose is called, and it is blocked in takeMVar, if an async exception arrives takeMVar will be interrupted and the file descriptor will never be closed. Arguably that situation shouldn't happen except in poorly-designed programs, but I can provide at least one example where it appears to be a viable architecture, and I'm not convinced it would never happen in practice.

So arguably uninterruptibleMask is not what we want for hClose.

Is there any way to fix the issue I describe besides preventing async exceptions from arising while blocked on the MVar? Although I do agree we don't want to put uninterruptibleMask inside hClose (long ramble at http://johnlato.blogspot.ca/2014/11/exception-handling-and-cleanup.html) John

Cheers, Simon

...

G

On Thu, Nov 20, 2014 at 7:24 AM, Simon Marlow mailto:marlowsd@gmail.com> wrote:

On 19/11/2014 23:07, Ganesh Sittampalam wrote:

On 13/11/2014 10:44, Simon Marlow wrote:

On 13/11/2014 07:47, Merijn Verstraaten wrote:

A new version would look like:

bracket before after thing = mask $ \restore -> do let atomicAfter = uninterruptibleMask . after a <- before r <- restore (thing a) `onException` atomicAfter a _ <- atomicAfter a return r

Slightly different versions are possible and the other relevant bracketing functions mentioned in this thread can be treated similarly.

Since we would need this for catch too, the sensible thing to do (if we decide to go ahead with this) would be to change the implementation of catch in the RTS from masking the exception handler to uninterruptibleMask. That would mean that at least for catch there would be no additional overhead, and it would make the modifications to the other operations simpler in some cases.

If this isn't done in the RTS, is there a possibility of an async exception slipping in between the exception handler starting and the uninterruptibleMask starting?

No, because the exception handler is masked.

Cheers, Simon

_________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

-- Gregory Collins

...

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On 23/11/14 17:46, John Lato wrote:

On Sun Nov 23 2014 at 7:09:10 AM Simon Marlow mailto:marlowsd@gmail.com> wrote:

On 21/11/14 17:22, Gregory Collins wrote: > New post from Yuras provides food for thought: > https://github.com/Yuras/io-__region/wiki/Handling-%28async%__29-exceptions-... https://github.com/Yuras/io-region/wiki/Handling-%28async%29-exceptions-in-h... > > In particular he points out a case for which uninterruptibleMask will > cause unkillable threads: let's say hClose blocks flushing the output to > a file, but the write fails because of a hardware error and blocks > forever. (Alternatively, imagine the file is on NFS and you get a cable > cut between the two machines). The thread executing hClose in the > cleanup action becomes unkillable.

Yes, and furthermore hClose is not "buggy": even if it is interrupted by an async exception, the file descriptor will still be closed by the finalizer. This is not something you want to do a lot, of course, but as a backup plan for the rare case of an async exception killing the cleanup action it's fine.

This is not entirely correct. If another thread is holding the MVar when hClose is called, and it is blocked in takeMVar, if an async exception arrives takeMVar will be interrupted and the file descriptor will never be closed. Arguably that situation shouldn't happen except in poorly-designed programs, but I can provide at least one example where it appears to be a viable architecture, and I'm not convinced it would never happen in practice.

Handles have a finalizer that closes the Handle sometime after its last use, so it's not possible to leak a Handle permanently. There's an unpredictable delay before it gets closed, which is why we don't rely on this to close our Handles normally, but I'm arguing it's fine to rely on this to close Handles in the rare case where hClose is interrupted.

So arguably uninterruptibleMask is not what we want for hClose.

Is there any way to fix the issue I describe besides preventing async exceptions from arising while blocked on the MVar? Although I do agree we don't want to put uninterruptibleMask inside hClose (long ramble at http://johnlato.blogspot.ca/2014/11/exception-handling-and-cleanup.html)

In the absence of a finalizer, besides uninterruptibleMask the only alternative that springs to mind is to fork a thread to finish the cleanup when an async exception strikes; which is actually not a bad solution as long as you don't rely on the resource being released in the original thread. Cheers, Simon

John

Cheers, Simon

> G > > On Thu, Nov 20, 2014 at 7:24 AM, Simon Marlow mailto:marlowsd@gmail.com > mailto:marlowsd@gmail.com>> wrote: > > On 19/11/2014 23:07, Ganesh Sittampalam wrote: > > On 13/11/2014 10:44, Simon Marlow wrote: > > On 13/11/2014 07:47, Merijn Verstraaten wrote: > > > A new version would look like: > > bracket before after thing = > mask $ \restore -> do > let atomicAfter = uninterruptibleMask . after > a <- before > r <- restore (thing a) `onException` atomicAfter a > _ <- atomicAfter a > return r > > Slightly different versions are possible and the other > relevant > bracketing functions mentioned in this thread can be > treated similarly. > > > Since we would need this for catch too, the sensible thing > to do (if we > decide to go ahead with this) would be to change the > implementation of > catch in the RTS from masking the exception handler to > uninterruptibleMask. That would mean that at least for > catch there > would be no additional overhead, and it would make the > modifications to > the other operations simpler in some cases. > > > If this isn't done in the RTS, is there a possibility of an async > exception slipping in between the exception handler starting and the > uninterruptibleMask starting? > > > No, because the exception handler is masked. > > Cheers, > Simon > > ___________________________________________________ > Libraries mailing list > Libraries@haskell.org mailto:Libraries@haskell.org mailto:Libraries@haskell.org> > http://www.haskell.org/____mailman/listinfo/libraries http://www.haskell.org/__mailman/listinfo/libraries > <http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries> > > > > > -- > Gregory Collins mailto:greg@gregorycollins.net mailto:greg@gregorycollins.net>>

_________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

On 23/11/14 20:23, Edward Kmett wrote:

This is predicated on a lot of assumptions though:

* That nobody else has closed over a reference to the handle in a way that prevents the GC from freeing it.

Yup

* That file handles are the only resources we are concerned with.

Oh no, I'm not assuming that - this corner of the discussion is just about hClose. Of course it doesn't extend to non-finalized resources.

* That eventual clean up is okay when things go wrong.

Well, I think this is not at all clear either way. Which would you rather have: - cleanup that might block in an unkillable way - cleanup performed asynchronously while the current thread remains killable. Clearly only the programmer can answer that, it depends on whether async cleanup is acceptable; if it is, this is almost certainly preferable, because we get to keep responsiveness.

These often hold, and at least we can say they hold for file handles for all but the most resource constrained of users, but I think ultimately there is a fundamental tension between real actual finalization guarantees and having killThread return in a timely manner when its target goes off the reservation.

With files we can get into situations where just bracketing "badly" still just works. With other resources though its a fair bit more hazardous.

Whether that warrants a change in the default form of bracket is another matter.

Indeed. To move this discussion forward, I would like to see someone catalogue common and existing use cases of bracket and document their buginess (or lack thereof) under the current and proposed semantics. Do it on a wiki page, then we'll have something concrete to argue over. I would do this myself but for lack of time, but I'll try to contribute to the discussion nonetheless. Cheers, Simon

-Edward

On Sun, Nov 23, 2014 at 1:57 PM, Simon Marlow mailto:marlowsd@gmail.com> wrote:

On 23/11/14 17:46, John Lato wrote:

On Sun Nov 23 2014 at 7:09:10 AM Simon Marlow mailto:marlowsd@gmail.com mailto:marlowsd@gmail.com>> wrote:

On 21/11/14 17:22, Gregory Collins wrote: > New post from Yuras provides food for thought: > https://github.com/Yuras/io-____region/wiki/Handling-%28async%____29-excepti... https://github.com/Yuras/io-__region/wiki/Handling-%28async%__29-exceptions-...

<https://github.com/Yuras/io-__region/wiki/Handling-%28async%__29-exceptions-... https://github.com/Yuras/io-region/wiki/Handling-%28async%29-exceptions-in-h...> > > In particular he points out a case for which uninterruptibleMask will > cause unkillable threads: let's say hClose blocks flushing the output to > a file, but the write fails because of a hardware error and blocks > forever. (Alternatively, imagine the file is on NFS and you get a cable > cut between the two machines). The thread executing hClose in the > cleanup action becomes unkillable.

Yes, and furthermore hClose is not "buggy": even if it is interrupted by an async exception, the file descriptor will still be closed by the finalizer. This is not something you want to do a lot, of course, but as a backup plan for the rare case of an async exception killing the cleanup action it's fine.

This is not entirely correct. If another thread is holding the MVar when hClose is called, and it is blocked in takeMVar, if an async exception arrives takeMVar will be interrupted and the file descriptor will never be closed. Arguably that situation shouldn't happen except in poorly-designed programs, but I can provide at least one example where it appears to be a viable architecture, and I'm not convinced it would never happen in practice.

Handles have a finalizer that closes the Handle sometime after its last use, so it's not possible to leak a Handle permanently. There's an unpredictable delay before it gets closed, which is why we don't rely on this to close our Handles normally, but I'm arguing it's fine to rely on this to close Handles in the rare case where hClose is interrupted.

So arguably uninterruptibleMask is not what we want for hClose.

Is there any way to fix the issue I describe besides preventing async exceptions from arising while blocked on the MVar? Although I do agree we don't want to put uninterruptibleMask inside hClose (long ramble at http://johnlato.blogspot.ca/__2014/11/exception-handling-__and-cleanup.html http://johnlato.blogspot.ca/2014/11/exception-handling-and-cleanup.html)

In the absence of a finalizer, besides uninterruptibleMask the only alternative that springs to mind is to fork a thread to finish the cleanup when an async exception strikes; which is actually not a bad solution as long as you don't rely on the resource being released in the original thread.

Cheers, Simon

John

Cheers, Simon

> G > > On Thu, Nov 20, 2014 at 7:24 AM, Simon Marlow mailto:marlowsd@gmail.com mailto:marlowsd@gmail.com> > mailto:marlowsd@gmail.com mailto:marlowsd@gmail.com>>> wrote: > > On 19/11/2014 23:07, Ganesh Sittampalam wrote: > > On 13/11/2014 10:44, Simon Marlow wrote: > > On 13/11/2014 07:47, Merijn Verstraaten wrote: > > > A new version would look like: > > bracket before after thing = > mask $ \restore -> do > let atomicAfter = uninterruptibleMask . after > a <- before > r <- restore (thing a) `onException` atomicAfter a > _ <- atomicAfter a > return r > > Slightly different versions are possible and the other > relevant > bracketing functions mentioned in this thread can be > treated similarly. > > > Since we would need this for catch too, the sensible thing > to do (if we > decide to go ahead with this) would be to change the > implementation of > catch in the RTS from masking the exception handler to > uninterruptibleMask. That would mean that at least for > catch there > would be no additional overhead, and it would make the > modifications to > the other operations simpler in some cases. > > > If this isn't done in the RTS, is there a possibility of an async > exception slipping in between the exception handler starting and the > uninterruptibleMask starting? > > > No, because the exception handler is masked. > > Cheers, > Simon > > _____________________________________________________ > Libraries mailing list > Libraries@haskell.org mailto:Libraries@haskell.org mailto:Libraries@haskell.org> mailto:Libraries@haskell.org mailto:Libraries@haskell.org>__> > http://www.haskell.org/______mailman/listinfo/libraries http://www.haskell.org/____mailman/listinfo/libraries <http://www.haskell.org/____mailman/listinfo/libraries http://www.haskell.org/__mailman/listinfo/libraries> > <http://www.haskell.org/____mailman/listinfo/libraries http://www.haskell.org/__mailman/listinfo/libraries <http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries>> > > > > > -- > Gregory Collins mailto:greg@gregorycollins.net mailto:greg@gregorycollins.net> mailto:greg@gregorycollins.____net mailto:greg@gregorycollins.net>>>

___________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org mailto:Libraries@haskell.org> http://www.haskell.org/____mailman/listinfo/libraries http://www.haskell.org/__mailman/listinfo/libraries <http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries>

_________________________________________________ Libraries mailing list Libraries@haskell.org mailto:Libraries@haskell.org http://www.haskell.org/__mailman/listinfo/libraries http://www.haskell.org/mailman/listinfo/libraries

On Fri, Nov 14, 2014 at 10:22 PM, Yuras Shumovich wrote:

Thank you for summarizing it, because the thread really run out of control (and it is my fault too, sorry for that)

I'm +1 for *Documentation Only* I'm +0.5 for *Combinators*

FYI in "library" subdirectory of ghc source tree I found 3 uses of uninterruptibleMask. Core libraries deal with low level detail often, but still only 3 uses, I'm surprised. Is it enough for default behavior.

They are: - base/Control/Concurrent/QSem.hs - base/Control/Concurrent/QSemN.hs - base/System/Timeout.hs

If there are places where uninterruptibleMask is missing in core libraries, then we should fix this bugs and then decide whether it is used frequently enough to switch defaults.

Note that for the non-interruptible cases, either would work, so you want to measure the frequency of the brackets that have interruptible cleanups. I think after adding the uninterruptibleMasks to all the needed brackets, and measuring their percentage of all brackets, you'll get a percentage somewhere around 100% :-)

...

It strikes me that this is winding up highly controversial.

Assuming that, well, things might not go your way in terms of getting bracket changed, let's think a bit about what a more retrenched solution would look like.

This way we have a continuum of fixes and can try to find the right point in the continuum.

So let's put forth a couple more colors for the bikeshed:

*Documentation Only*

The issue is that users use bracket assuming things will be safer than

...

are. At the very least we need to clearly document the fact that this isn't the case!

We could document the pattern of using uninterruptibleMask_ yourself in handlers for safety and include examples of where it is required.

*Combinators*

If we want to go further, we could introduce:

uninterruptibleBracket :: IO a -> (a -> IO b) -> (a -> IO c) -> IO c uninterruptibleBracket acquire release = bracket acquire (uninterruptibleMask_ . release)

uninterruptibleBracket_ :: IO a -> IO b -> IO c -> IO c uninterruptibleBracket_ acquire release = bracket_ acquire (uninterruptibleMask_ release)

etc.

and then upgrade the documentation for `bracket` and the like with a big fat warning about how certain common sense examples using bracket should really be using uninterruptibleBracket.

*Merijn's Proposal*

We could go further and switch the default behavior of bracket to that of uninterruptibleBracket above. Then we get Merijn's proposal.

*Going Further*

But if we do that we should probably consider adding an `interruptibleBracket` that matches the existing behavior with the same caveats we would want to put on `bracket`.

The use cases that folks have that center around weird RPC handling scenarios in the release handler seem to fit into this niche.

This redefines bracket, like Merijn would prefer, because it is a source of very very hard to track down resource bugs, and make the few who actually want to do complex code that relies on active asynchronous exception support in the handler switch combinators.

It also has the benefit that the name interruptibleBracket is easier to explain than uninterruptibleBracket, which only made release uninterruptible.

The kind of code that would be affected is the kind of code that would be very visibly affected, whereas the kind of code that is broken right now is scattered across the entire ecosystem and is just subtly wrong.

*Personal Thoughts*

I started writing this proposal with a continuum in mind, thinking I'd land somewhere in the middle.

Normally, I'd be disinclined to change semantics on a function with such widespread use! I very strongly sympathize with Greg's position here.

However, personally, I think the "Going Further" solution above is the right solution, which is effectively Merijn's proposal with the addition of interruptibleBracket and the like.

However, in this case the only code that really can rely upon this behavior is code that happens to know it won't kill the thread from outside until it is in the handler, but that they want to do a thing that will throw them asynchronous exceptions _within_ the handler, and well, that is a marginal enough use case that I don't have much a problem marginalizing it further by forcing its practitioners to use a more exotic combinator. If they are absolutely allergic to the new semantics you can always swap all the existing uses of bracket to interruptibleBracket, and the very kind of user who would need these semantics is the kind of user who is equipped to carry out this sort of change.

This effectively puts me at +1 with the caveat that I'd like to see interruptibleBracket added.

-Edward

On Tue, Nov 11, 2014 at 1:09 PM, Merijn Verstraaten < merijn@inconsistent.nl> wrote:

...

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds

...

...

"deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however

...

...

also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas

On Fri, 2014-11-14 at 13:30 -0500, Edward Kmett wrote: they that this there's

...

...

many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

-- Eyal

+1 to the edward kmett clarified proposal. On Fri, Nov 14, 2014 at 5:17 PM, Merijn Verstraaten wrote:

Hi Edward,

I think both me and Eyal where taking the addition of "interruptibleBracket" and variants for granted as an obvious part of this proposal. I fully agree that there *should* be an interruptibleBracket for people who want it. I just think it's the wrong default and that 'bracket' should become the obvious default.

-- Merijn

On Fri, Nov 14, 2014, at 10:30, Edward Kmett wrote:

It strikes me that this is winding up highly controversial.

Assuming that, well, things might not go your way in terms of getting bracket changed, let's think a bit about what a more retrenched solution would look like.

This way we have a continuum of fixes and can try to find the right point in the continuum.

So let's put forth a couple more colors for the bikeshed:

*Documentation Only*

The issue is that users use bracket assuming things will be safer than they are. At the very least we need to clearly document the fact that this isn't the case!

We could document the pattern of using uninterruptibleMask_ yourself in handlers for safety and include examples of where it is required.

*Combinators*

If we want to go further, we could introduce:

uninterruptibleBracket :: IO a -> (a -> IO b) -> (a -> IO c) -> IO c uninterruptibleBracket acquire release = bracket acquire (uninterruptibleMask_ . release)

uninterruptibleBracket_ :: IO a -> IO b -> IO c -> IO c uninterruptibleBracket_ acquire release = bracket_ acquire (uninterruptibleMask_ release)

etc.

and then upgrade the documentation for `bracket` and the like with a big fat warning about how certain common sense examples using bracket should really be using uninterruptibleBracket.

*Merijn's Proposal*

We could go further and switch the default behavior of bracket to that of uninterruptibleBracket above. Then we get Merijn's proposal.

*Going Further*

But if we do that we should probably consider adding an `interruptibleBracket` that matches the existing behavior with the same caveats we would want to put on `bracket`.

The use cases that folks have that center around weird RPC handling scenarios in the release handler seem to fit into this niche.

This redefines bracket, like Merijn would prefer, because it is a source of very very hard to track down resource bugs, and make the few who actually want to do complex code that relies on active asynchronous exception support in the handler switch combinators.

It also has the benefit that the name interruptibleBracket is easier to explain than uninterruptibleBracket, which only made release uninterruptible.

The kind of code that would be affected is the kind of code that would be very visibly affected, whereas the kind of code that is broken right now is scattered across the entire ecosystem and is just subtly wrong.

*Personal Thoughts*

I started writing this proposal with a continuum in mind, thinking I'd land somewhere in the middle.

Normally, I'd be disinclined to change semantics on a function with such widespread use! I very strongly sympathize with Greg's position here.

However, personally, I think the "Going Further" solution above is the right solution, which is effectively Merijn's proposal with the addition of interruptibleBracket and the like.

However, in this case the only code that really can rely upon this behavior is code that happens to know it won't kill the thread from outside until it is in the handler, but that they want to do a thing that will throw them asynchronous exceptions _within_ the handler, and well, that is a marginal enough use case that I don't have much a problem marginalizing it further by forcing its practitioners to use a more exotic combinator. If they are absolutely allergic to the new semantics you can always swap all the existing uses of bracket to interruptibleBracket, and the very kind of user who would need these semantics is the kind of user who is equipped to carry out this sort of change.

This effectively puts me at +1 with the caveat that I'd like to see interruptibleBracket added.

-Edward

On Tue, Nov 11, 2014 at 1:09 PM, Merijn Verstraaten < merijn@inconsistent.nl> wrote:

Ola!

In September Eyal Lotem raised the issue of bracket's cleanup handler not being uninterruptible [1]. This is a final bikeshedding email before I submit a patch.

The problem, summarised: Blocking cleanup actions can be interrupted, causing cleanup not to happen and potentially leaking resources.

Main objection to making the cleanup handler uninterruptible: Could cause deadlock if the code relies on async exceptions to interrupt a blocked thread.

I count only two objections in the previous thread, 1 on the grounds that "deadlocks are NOT unlikely" and 1 that is conditioned on "I don't believe this is a problem".

The rest seems either +1, or at least agrees that the status quo is *worse* than the proposed solution.

My counter to these objections is: 1) No one has yet shown me any code that relies on the cleanup handler being interruptible

2) There are plenty of examples of current code being broken, for example every single 'bracket' using file handles is broken due to handle operations using a potentially blocking MVar operation internally, potentially leaking file descriptors/handles.

3) Even GHC-HQ can't use bracket correctly (see Simon's emails)

Potential solution #1: Leave bracket as-is, add bracketUninterruptible with an uninterruptible cleanup handler.

Potential solution #2: Change bracket to use uninterruptible cleanup handler, add bracketInterruptible for interruptible cleanups.

Trade-offs: Solution 1 won't change the semantics of any existing code, however this also means that any currently broken uses of bracket will remain broken, possibly indefinitely.

Solution 2 will change the semantics of bracket, which means any currently broken uses of bracket will be fixed, at the cost of creating potential deadlocks in code that relies on the interruptibility of cleanup.

I will argue that solution #2 is preferable, since I have yet to see any code that uses the interruptibility of the cleanup handler. Whereas there's many broken assumption assuming the cleanup handler is not interruptible.

Secondly, it is easier to detect deadlocks caused by this problem than it is to detect resource leaks which only happen in unlucky timings of async exceptions. Especially since any deadlock caused by the change can be fixed by replacing bracket with bracketInterruptible.

[1] - https://www.haskell.org/pipermail/libraries/2014-September/023675.html

Cheers, Merijn _______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries

On 17/11/14 11:05 AM, Mark Lentczner wrote:

Hmmmmmmmmmmmmm...... That was a slog! :-)

Indeed. +1 for Merijn's proposal from me. I have no opinion on interruptibleBracket. BTW, I don't feel technically qualified to vote on this, so feel free to qualify my +1 by a factor of 0.1 or so. The only reasons I'm voting are: - I feel bloody entitled after reading through the entire thread, and - I don't want any C++ programmers to be able to point to a Haskell core library as an example of a broken design.