From 64ed3311d1b31f9b795327675c746f705d3c7646 Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Thu, 30 Jan 2025 15:32:38 +0300 Subject: [PATCH 1/9] Add test for checking that runPyInMain doesn't deadlock If exception is raised. It does. --- test/TST/Run.hs | 7 ++++++- test/TST/Util.hs | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/test/TST/Run.hs b/test/TST/Run.hs index ed547c6..c4ea7b1 100644 --- a/test/TST/Run.hs +++ b/test/TST/Run.hs @@ -19,7 +19,12 @@ tests = testGroup "Run python" import threading assert threading.main_thread() == threading.current_thread() |] - , testCase "Python exceptions are converted" $ runPy $ throwsPy [py_| 1 / 0 |] + , testCase "Python exceptions are converted (py)" $ runPy $ throwsPy [py_| 1 / 0 |] + , testCase "Python exceptions are converted (std)" $ throwsPyIO $ runPy [py_| 1 / 0 |] + , testCase "Python exceptions are converted (main)" $ throwsPyIO $ runPyInMain [py_| 1 / 0 |] + , testCase "Main doesn't deadlock after exception" $ do + throwsPyIO $ runPyInMain [py_| 1 / 0 |] + runPyInMain [py_| assert True |] , testCase "Scope pymain->any" $ runPy $ do [pymain| x = 12 diff --git a/test/TST/Util.hs b/test/TST/Util.hs index 8295cd4..df918e8 100644 --- a/test/TST/Util.hs +++ b/test/TST/Util.hs @@ -12,3 +12,7 @@ throwsPy :: Py () -> Py () throwsPy io = (io >> liftIO (assertFailure "Evaluation should raise python exception")) `catch` (\(_::PyError) -> pure ()) +throwsPyIO :: IO () -> IO () +throwsPyIO io = (io >> assertFailure "Evaluation should raise python exception") + `catch` (\(_::PyError) -> pure ()) + From 51946952ef40d39547f2f55d5aa740e0a0522a85 Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Thu, 30 Jan 2025 16:33:31 +0300 Subject: [PATCH 2/9] Exception from python should not trigger InterruptMain This resulted in deadlock. And it looks like InterruptMain is completely wrong as well. --- src/Python/Internal/Eval.hs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index c46b097..390db3d 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -474,11 +474,12 @@ runPyInMain py RunningN _ eval tid_main _ -> do acquireLock tid_main pure - $ flip finally (atomically (releaseLock tid_main)) - $ flip onException (throwTo tid_main InterruptMain) - $ do resp <- newEmptyMVar - putMVar eval $ EvalReq py resp - either throwM pure =<< takeMVar resp + $ flip finally (atomically (releaseLock tid_main)) + $ do r <- flip onException (throwTo tid_main InterruptMain) + $ do resp <- newEmptyMVar + putMVar eval $ EvalReq py resp + takeMVar resp + either throwM pure r -- Single-threaded RTS | otherwise = runPy py From cc9e2e3f5d75406d421617f9d0c5b51ab5bcc4eb Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 21:55:42 +0300 Subject: [PATCH 3/9] Add test for interrupting runPyMain with exception --- test/TST/Run.hs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/TST/Run.hs b/test/TST/Run.hs index c4ea7b1..fef8c06 100644 --- a/test/TST/Run.hs +++ b/test/TST/Run.hs @@ -2,6 +2,8 @@ -- Tests for variable scope and names module TST.Run(tests) where +import Control.Concurrent +import Control.Exception import Control.Monad import Control.Monad.IO.Class import Test.Tasty @@ -25,6 +27,18 @@ tests = testGroup "Run python" , testCase "Main doesn't deadlock after exception" $ do throwsPyIO $ runPyInMain [py_| 1 / 0 |] runPyInMain [py_| assert True |] + -- Here we test that exceptions are really passed to python's thread without running python + , testCase "Exception in runPyInMain works" $ do + lock <- newEmptyMVar + tid <- myThreadId + _ <- forkIO $ takeMVar lock >> throwTo tid Stop + handle (\Stop -> pure ()) + $ runPyInMain + $ do liftIO $ putMVar lock () + liftIO $ threadDelay 10_000_000 + error "Should be interrupted" + runPyInMain $ pure () + -- , testCase "Scope pymain->any" $ runPy $ do [pymain| x = 12 @@ -117,3 +131,7 @@ tests = testGroup "Run python" pass |] ] + +data Stop = Stop + deriving stock Show + deriving anyclass Exception From c5de4b44cb1c6cf04395fa5bf5568bab494013ae Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 22:12:32 +0300 Subject: [PATCH 4/9] Main python's thread need not to handle InterruptMain specially It will simply put exception into response MVar which will be GC'd soon --- src/Python/Internal/Eval.hs | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index 390db3d..9522707 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -41,6 +41,7 @@ import Control.Monad.Catch import Control.Monad.IO.Class import Control.Monad.Trans.Cont import Data.Maybe +import Data.Function import Foreign.Concurrent qualified as GHC import Foreign.Ptr import Foreign.ForeignPtr @@ -335,22 +336,18 @@ mainThread lock_init lock_eval = do putMVar lock_init r_init case r_init of False -> pure () - True -> mask_ $ do - let loop - = handle (\InterruptMain -> pure ()) - $ takeMVar lock_eval >>= \case - EvalReq py resp -> do - res <- (Right <$> runPy py) `catch` (pure . Left) - putMVar resp res - loop - StopReq resp -> do - [C.block| void { - PyGILState_Ensure(); - Py_Finalize(); - } |] - putMVar resp () - loop - + True -> mask_ $ fix $ \loop -> + takeMVar lock_eval >>= \case + EvalReq py resp -> do + res <- (Right <$> runPy py) `catch` (pure . Left) + putMVar resp res + loop + StopReq resp -> do + [C.block| void { + PyGILState_Ensure(); + Py_Finalize(); + } |] + putMVar resp () doInializePythonIO :: IO Bool From 5850166657047de240841c3bea19a79c5afea362 Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 22:23:00 +0300 Subject: [PATCH 5/9] Correctly implement exception handling in runPyInMain - Use bracket for taking/releasing lock - Only wait for interrupt exception on return value MVar --- src/Python/Internal/Eval.hs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index 9522707..410f175 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -461,7 +461,11 @@ runPyInMain :: Py a -> IO a -- See NOTE: [Python and threading] runPyInMain py -- Multithreaded RTS - | rtsSupportsBoundThreads = join $ atomically $ readTVar globalPyState >>= \case + | rtsSupportsBoundThreads = bracket acquireMain releaseMain evalMain + -- Single-threaded RTS + | otherwise = runPy py + where + acquireMain = atomically $ readTVar globalPyState >>= \case NotInitialized -> throwSTM PythonNotInitialized InitFailed -> throwSTM PyInitializationFailed Finalized -> throwSTM PythonIsFinalized @@ -470,15 +474,15 @@ runPyInMain py Running1 -> throwSTM $ PyInternalError "runPyInMain: Running1" RunningN _ eval tid_main _ -> do acquireLock tid_main - pure - $ flip finally (atomically (releaseLock tid_main)) - $ do r <- flip onException (throwTo tid_main InterruptMain) - $ do resp <- newEmptyMVar - putMVar eval $ EvalReq py resp - takeMVar resp - either throwM pure r - -- Single-threaded RTS - | otherwise = runPy py + pure (tid_main, eval) + -- + releaseMain (tid_main, _ ) = atomically (releaseLock tid_main) + evalMain (tid_main, eval) = do + r <- mask_ $ do resp <- newEmptyMVar + putMVar eval $ EvalReq py resp + takeMVar resp `onException` throwTo tid_main InterruptMain + either throwM pure r + -- | Execute python action. This function is unsafe and should be only -- called in thread of interpreter. From d0b446359e051bf4e11c8e5d8decebe3121fc2db Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 22:52:05 +0300 Subject: [PATCH 6/9] Rename unPy -> unsafeRunPy --- src/Python/Inline/Literal.hs | 2 +- src/Python/Internal/Eval.hs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Python/Inline/Literal.hs b/src/Python/Inline/Literal.hs index 5a4f472..72fbbbd 100644 --- a/src/Python/Inline/Literal.hs +++ b/src/Python/Inline/Literal.hs @@ -607,7 +607,7 @@ instance (FromPy a1, FromPy a2, ToPy b) => ToPy (a1 -> a2 -> IO b) where -- | Execute haskell callback function pyCallback :: Program (Ptr PyObject) (Ptr PyObject) -> IO (Ptr PyObject) -pyCallback io = callbackEnsurePyLock $ unPy $ ensureGIL $ runProgram io `catch` convertHaskell2Py +pyCallback io = callbackEnsurePyLock $ unsafeRunPy $ ensureGIL $ runProgram io `catch` convertHaskell2Py -- | Load argument from python object for haskell evaluation loadArg diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index 410f175..7c9c99e 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -15,7 +15,7 @@ module Python.Internal.Eval -- * Evaluator , runPy , runPyInMain - , unPy + , unsafeRunPy -- * GC-related , newPyObject -- * C-API wrappers @@ -451,7 +451,7 @@ runPy py where -- We check whether interpreter is initialized. Throw exception if -- it wasn't. Better than segfault isn't it? - go = ensurePyLock $ unPy (ensureGIL py) + go = ensurePyLock $ unsafeRunPy (ensureGIL py) -- | Same as 'runPy' but will make sure that code is run in python's -- main thread. It's thread in which python's interpreter was @@ -486,8 +486,8 @@ runPyInMain py -- | Execute python action. This function is unsafe and should be only -- called in thread of interpreter. -unPy :: Py a -> IO a -unPy (Py io) = io +unsafeRunPy :: Py a -> IO a +unsafeRunPy (Py io) = io From 132686087e8f4c9302866036f72a4817f60bed3b Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 22:57:06 +0300 Subject: [PATCH 7/9] We don't need mask_ during python finalization --- src/Python/Internal/Eval.hs | 57 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index 7c9c99e..dabba78 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -280,7 +280,33 @@ initializePython = [CU.exp| int { Py_IsInitialized() } |] >>= \case -- | Destroy python interpreter. finalizePython :: IO () -finalizePython = mask_ doFinalizePython +finalizePython = join $ atomically $ readTVar globalPyState >>= \case + NotInitialized -> throwSTM PythonNotInitialized + InitFailed -> throwSTM PythonIsFinalized + Finalized -> pure $ pure () + InInitialization -> retry + InFinalization -> retry + -- We can simply call Py_Finalize + Running1 -> checkLock $ [C.block| void { + PyGILState_Ensure(); + Py_Finalize(); + } |] + -- We need to call Py_Finalize on main thread + RunningN _ eval _ tid_gc -> checkLock $ do + killThread tid_gc + resp <- newEmptyMVar + putMVar eval $ StopReq resp + takeMVar resp + where + checkLock action = readTVar globalPyLock >>= \case + LockUninialized -> throwSTM $ PyInternalError "finalizePython LockUninialized" + LockFinalized -> throwSTM $ PyInternalError "finalizePython LockFinalized" + Locked{} -> retry + LockedByGC -> retry + LockUnlocked -> do + writeTVar globalPyLock LockFinalized + writeTVar globalPyState Finalized + pure action -- | Bracket which ensures that action is executed with properly -- initialized interpreter @@ -398,35 +424,6 @@ doInializePythonIO = do } |] return $! r == 0 -doFinalizePython :: IO () -doFinalizePython = join $ atomically $ readTVar globalPyState >>= \case - NotInitialized -> throwSTM PythonNotInitialized - InitFailed -> throwSTM PythonIsFinalized - Finalized -> pure $ pure () - InInitialization -> retry - InFinalization -> retry - -- We can simply call Py_Finalize - Running1 -> checkLock $ [C.block| void { - PyGILState_Ensure(); - Py_Finalize(); - } |] - -- We need to call Py_Finalize on main thread - RunningN _ eval _ tid_gc -> checkLock $ do - killThread tid_gc - resp <- newEmptyMVar - putMVar eval $ StopReq resp - takeMVar resp - where - checkLock action = readTVar globalPyLock >>= \case - LockUninialized -> throwSTM $ PyInternalError "doFinalizePython LockUninialized" - LockFinalized -> throwSTM $ PyInternalError "doFinalizePython LockFinalized" - Locked{} -> retry - LockedByGC -> retry - LockUnlocked -> do - writeTVar globalPyLock LockFinalized - writeTVar globalPyState Finalized - pure action - ---------------------------------------------------------------- -- Running Py monad From 6567d6757a2924efb80eeb663b76f9ff85f37ee7 Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 23:00:34 +0300 Subject: [PATCH 8/9] Python initialization does not need mask_ --- src/Python/Internal/Eval.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index dabba78..ae64938 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -274,8 +274,8 @@ releaseLock tid = readTVar globalPyLock >>= \case initializePython :: IO () -- See NOTE: [Python and threading] initializePython = [CU.exp| int { Py_IsInitialized() } |] >>= \case - 0 | rtsSupportsBoundThreads -> runInBoundThread $ mask_ $ doInializePython - | otherwise -> mask_ $ doInializePython + 0 | rtsSupportsBoundThreads -> runInBoundThread $ doInializePython + | otherwise -> doInializePython _ -> pure () -- | Destroy python interpreter. From fe21e291af8a235f9233bb1bc59ed412a2aebab5 Mon Sep 17 00:00:00 2001 From: Alexey Khudyakov Date: Sat, 1 Feb 2025 23:05:23 +0300 Subject: [PATCH 9/9] Ensure that Py is masked --- src/Python/Internal/Eval.hs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Python/Internal/Eval.hs b/src/Python/Internal/Eval.hs index ae64938..8389ca4 100644 --- a/src/Python/Internal/Eval.hs +++ b/src/Python/Internal/Eval.hs @@ -330,7 +330,6 @@ doInializePython = do let fini st = atomically $ do writeTVar globalPyState $ st writeTVar globalPyLock $ LockUnlocked - pure $ (mask_ $ if -- On multithreaded runtime create bound thread to make @@ -448,7 +447,7 @@ runPy py where -- We check whether interpreter is initialized. Throw exception if -- it wasn't. Better than segfault isn't it? - go = ensurePyLock $ unsafeRunPy (ensureGIL py) + go = ensurePyLock $ mask_ $ unsafeRunPy (ensureGIL py) -- | Same as 'runPy' but will make sure that code is run in python's -- main thread. It's thread in which python's interpreter was