Improve catching of Ctrl-C and logging of Async exceptions in UCM #2498
Conversation
| interruptHandler (asyncExceptionFromException -> Just UserInterrupt) = awaitInput | ||
| interruptHandler e = error (show e) | ||
| interruptHandler e = putStrLn ("Exception: " <> show e) *> throwIO e |
There was a problem hiding this comment.
The previous error wasn't actually rendering the exception for some reason, perhaps because we weren't catching async exceptions, but I think re-throwing is probably more correct here regardless.
| @@ -170,9 +176,9 @@ main dir welcome initialPath (config, cancelConfig) initialInputs runtime codeba | |||
| e | |||
| x -> do | |||
| writeIORef pageOutput True | |||
| pure x) `catch` interruptHandler | |||
| pure x) `catchSyncOrAsync` interruptHandler | |||
There was a problem hiding this comment.
We need to be careful when catching async exceptions; should be okay here as long as we're sure to rethrow all exceptions other than UserInterrupt
| -- Run the main program loop, always run cleanup, | ||
| -- If an exception occurred, print it before exiting. | ||
| (loop (HandleInput.loopState0 root initialPath) | ||
| `withException` \e -> putStrLn ("Exce 10000 ption: " <> show (e :: SomeException))) |
There was a problem hiding this comment.
We were silently exiting without printing errors or exceptions before, this prints the exception, then re-throws, which then runs the finally
| where | ||
| -- catch ctrl-c and simply re-render the prompt. | ||
| haskelineCtrlCHandling act = |
There was a problem hiding this comment.
Suggestion:
haskelineCtrlCHandling act =
forever do
Line.handleInterrupt
(Line.outputStrLn "Interrupted.")
(Line.withInterrupt act)Otherwise, each invocation of the action after the first would run with asynchronous exceptions (interruptibly) masked, as that is the behavior of catch/handle.
There was a problem hiding this comment.
Also: perhaps we don't want to even print "Interrupted"? That's the behavior of ghci, anyway.
There was a problem hiding this comment.
I can't use forever since we need to return the input on a success; but switched to return a maybe and then check the result outside of the masked section instead 👍🏼
There was a problem hiding this comment.
For a while at least, we were trying to use friendly complete sentences instead of the more commonplace robotic computer messages. I couldn't come up with an alternative suggestion I liked better than "Interrupted.", but wanted to put the thought out there.
| @@ -44,6 +44,8 @@ import qualified Unison.CommandLine.Welcome as Welcome | |||
| import Text.Regex.TDFA | |||
| import Control.Lens (view) | |||
| import Control.Error (rightMay) | |||
| import UnliftIO (catchSyncOrAsync, throwIO, withException) | |||
There was a problem hiding this comment.
Maybe in a follow-up PR we could replace Control.Exceptiom with UnliftIO.Exception in this module, and maybe beyond
Since they have different semantics, it's confusing to have both in use.
Overview
Previously ctrl-c on the UCM prompt would just exit, which is really annoying and out-of-line with how shells typically handle things. In most shells, ctrl-c simply clears the prompt and re-prompts. This PR makes that the default behaviour.
You can still exit the CLI using
ctrl-d.Before:
After:
Implementation notes
catchSyncOrAsyncto allow it to catch User Interrupts.Interesting/controversial decisions
We have to be careful using
catchSyncOrAsync, using it incorrectly can make a process unkillable. It's important that we rethrow on all exceptions OTHER than UserInterruptWe might not actually need the
catchSyncOrAsyncif the haskeline catch is enough.Loose ends
A ctrl-c during intensive operations (e.g. if you run a slow
findcommand and ctrl-c, UCM will still exit with an async exception).I tried to catch this in multiple spots, but it seems that some async operation inside UCM is converting UserInterrupt into an
AsyncCancelledsomewhere. I'd like to track that down eventually, but this is an incremental improvement for now.