Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cancel in exception handling code #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion unliftio/src/UnliftIO/Exception.hs
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ withException thing after = withRunInIO $ \run -> EUnsafe.uninterruptibleMask $
case res1 of
Left e1 -> do
-- see explanation in bracket
_ :: Either SomeException b <- EUnsafe.try $ run $ after e1
_ :: Either SomeException b <- EUnsafe.try $ restore $ run $ after e1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this line fixes the test. But it also removes the guarantee that action `onException` cleanup will definitely complete cleanup.

EUnsafe.throwIO e1
Right x -> return x

Expand Down
23 changes: 22 additions & 1 deletion unliftio/test/UnliftIO/ExceptionSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module UnliftIO.ExceptionSpec (spec) where

import qualified Control.Exception
import Control.Monad (void, (<=<))
import Control.Monad (void, (<=<), when)
import Data.Bifunctor (first)
import Test.Hspec
import UnliftIO
Expand Down Expand Up @@ -79,6 +79,27 @@ spec = do
result <- withWrappedAsyncExceptionThrown $ \m -> trySyncOrAsync (void m)
first fromExceptionUnwrap result `shouldBe` Left (Just Exception1)

describe "withException" $ do
it "should work when withAsync is in the handler" $ do
let
action =
error "oops"
`onException` do
let
timerAction n = do
threadDelay 1000000
when (n < 10) $ do
timerAction (n + 1)
withAsync (timerAction 0) $ \a -> do
cancel a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be replaced with pure (). withAsync is also unable to cancel the thread.

eresult <-
race
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This race call also is unable to cancel one of the threads - so, with the original code, this test hangs for 10 seconds waiting for timerAction to complete before returning Right 10.

(action `shouldThrow` errorCall "oops")
(do
threadDelay 1000000
pure 10)
eresult `shouldBe` Left ()

describe "fromExceptionUnwrap" $ do
it "should be the inverse of toAsyncException" $ do
fromExceptionUnwrap (toAsyncException Exception1) `shouldBe` Just Exception1
Expand Down