From a588ba3bfb5673665179f5fbeb2e4faf4a0375ae Mon Sep 17 00:00:00 2001 From: Guillaume Bouchard Date: Tue, 13 Aug 2024 19:29:33 +0400 Subject: [PATCH] feat: do not catch async exception as test failure This is related to https://github.com/NorfairKing/sydtest/issues/80 The current exception handler for test was incorrectly catching async exception (the handler for `AsyncException` was not enough, the "parent" of the async exception hierarchy is `SomeAsyncException`). This lead to surprising behaviors with ctrl-C. when receiving a `ctrl-c` (e.g. an interruption of the test-suite by the user), the test runner was cancelled, cancelling the print loop of the test and cancelling all workers. However, the exception was caught in workers as a "test-failure", leading to it being ignored and the different tests retrieved and meaning that the test runner survived the interruption. They will hence continue their life (e.g. purging the job queue). It was sometime leading to a lot of delay before ending the process, or even sometime other errors related to the different involved `MVar` (when no producer / consumer detection detects the error. Note that a second ctrl-c would cancels the program execution completely (Because that's how the RTS behaves). HOWEVER, the behavior is completely different in the repl, because the repl won't terminate itself on second ctrl-C. Hence the worker threads are leaked and continues running, and even survives the repl reloads. In the best case, that's a waste of resources and CPU time, but if you happen to rerun new sydtest, it can stacks and some tests may run concurrently with the "ghost" version of themself, which may or may not work ;) --- sydtest-test/sydtest-test.cabal | 1 + sydtest-test/test/Test/Syd/ExceptionSpec.hs | 39 +++++++++++++++++++++ sydtest/CHANGELOG.md | 5 +++ sydtest/src/Test/Syd/Run.hs | 9 +++-- 4 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 sydtest-test/test/Test/Syd/ExceptionSpec.hs diff --git a/sydtest-test/sydtest-test.cabal b/sydtest-test/sydtest-test.cabal index 1527dfb..8e02f7a 100644 --- a/sydtest-test/sydtest-test.cabal +++ b/sydtest-test/sydtest-test.cabal @@ -88,6 +88,7 @@ test-suite sydtest-test Test.Syd.AroundSpec Test.Syd.DescriptionsSpec Test.Syd.DiffSpec + Test.Syd.ExceptionSpec Test.Syd.ExpectationSpec Test.Syd.FootgunSpec Test.Syd.GoldenSpec diff --git a/sydtest-test/test/Test/Syd/ExceptionSpec.hs b/sydtest-test/test/Test/Syd/ExceptionSpec.hs new file mode 100644 index 0000000..6835ca1 --- /dev/null +++ b/sydtest-test/test/Test/Syd/ExceptionSpec.hs @@ -0,0 +1,39 @@ +{-# LANGUAGE NumericUnderscores #-} + +module Test.Syd.ExceptionSpec where + +import Control.Concurrent (threadDelay) +import Control.Concurrent.Async +import Control.Exception (AsyncException, try) +import Control.Exception.Base (throwIO) +import Data.Time.Clock +import Test.Syd +import Test.Syd.OptParse (defaultSettings) + +spec = describe "exception handling" $ do + it "stops immediatly with async" $ do + -- Tests that when an async exception is sent to sydTest (for example, + -- ctrl-c), it behaves as expected and terminates immediately and does not + -- do anything surprising. + startTime <- liftIO getCurrentTime + + -- Runs two threads, one will be done in 1s, and the other is a test suite + -- with one test which should run for 10s. + -- When the first threads terminate, it will throw AsyncCancel in the + -- sydTest test suite, which should terminate asap. + -- This will be checked using the timer t. That's fragile, but we know that: + -- + -- t should be more than 100ms (because of the first threadDelay) + -- t should be no much more than 100ms. Especially, it should not be 10s + -- (waiting for the complete threadDelay in the test suite) or even more if + -- the exception is completly ignored (or test is retried) + race (threadDelay 100_000) $ do + sydTest $ do + it "is a dumb slow test" $ do + threadDelay 10_000_000 + + endTime <- liftIO getCurrentTime + + -- Less than 1 second + (endTime `diffUTCTime` startTime) `shouldSatisfy` (< 1) + pure () diff --git a/sydtest/CHANGELOG.md b/sydtest/CHANGELOG.md index 3cf51d8..42c359d 100644 --- a/sydtest/CHANGELOG.md +++ b/sydtest/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +### Changed + +- sydtest won't crash anymore or behave weirdly or leak ressource in repl when + interrupted by ctrl-C. See discussion here: https://github.com/NorfairKing/sydtest/issues/80#issuecomment-2286517212 + ## [0.17.0.0] - 2024-08-04 ### Changed diff --git a/sydtest/src/Test/Syd/Run.hs b/sydtest/src/Test/Syd/Run.hs index ad86c68..5fd23c6 100644 --- a/sydtest/src/Test/Syd/Run.hs +++ b/sydtest/src/Test/Syd/Run.hs @@ -374,8 +374,13 @@ runGoldenTestWithArg createGolden TestRunSettings {..} _ wrapper = do exceptionHandlers :: [Handler (Either SomeException a)] exceptionHandlers = - [ -- Re-throw AsyncException, otherwise execution will not terminate on SIGINT (ctrl-c). - Handler (\e -> throwIO (e :: AsyncException)), + [ -- Re-throw SomeAsyncException, otherwise execution will not terminate on SIGINT (ctrl-c). + -- This is also critical for correctness, because library such as async + -- uses this signal for `concurrently`, and `race`, ..., and if we ignore + -- this exception, we can end in a context where half of the logic has + -- stopped and yet we continue. + -- See https://github.com/NorfairKing/sydtest/issues/80 + Handler (\e -> throwIO (e :: SomeAsyncException)), -- Catch all the rest Handler (\e -> return $ Left (e :: SomeException)) ]