Skip to content

Commit

Permalink
feat: do not catch async exception as test failure
Browse files Browse the repository at this point in the history
This is related to NorfairKing#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 ;)
  • Loading branch information
guibou committed Sep 22, 2024
1 parent 23baea5 commit a588ba3
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 2 deletions.
1 change: 1 addition & 0 deletions sydtest-test/sydtest-test.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions sydtest-test/test/Test/Syd/ExceptionSpec.hs
Original file line number Diff line number Diff line change
@@ -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 ()
5 changes: 5 additions & 0 deletions sydtest/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
9 changes: 7 additions & 2 deletions sydtest/src/Test/Syd/Run.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
]
Expand Down

0 comments on commit a588ba3

Please sign in to comment.