Modify displayed exception type #261
Comments
|
I don't think "Terminated with exception" makes sense in We could add "terminated with exception" to the start of the message printed by the default handler, before the exception gets rendered at all, but am I right in thinking that even the default uncaught exception handler won't be followed by termination if the exception occurred on a non-main thread? |
Good point.
Ah good catch, you are right. The following program: main :: IO ()
main = do
putStrLn "main: before fork"
_ <- forkIO throwEx
putStrLn "main: after fork"
threadDelay 1_000_000
putStrLn "after delay"
where
throwEx = do
putStrLn "in throwEx"
throwIO $
IOError
{ ioe_handle = Nothing,
ioe_type = OtherError,
ioe_location = "throwEx",
ioe_description = "Some error",
ioe_errno = Nothing,
ioe_filename = Nothing
}prints: So probably "Termination" is not a good idea. Perhaps a header like |
|
I agree that "Uncaught exception" has a nicer ring to it, though an "uncaught" exception is one that, by definition, terminates the program, no? In any case I view this as a positive change. I personally would prefer the name of the exception to be printed before the backtraces, as I find they can sometimes get quite long! |
I think it is debatable (directly uncaught by the user vs. the rts), but you raise a good point that this could run up against reasonable intuition. Anticipating all use-cases is tricky! I agree that moving the type metadata before the stacktraces is the most important part of this change. As to a potential header in the handler, maybe the least likely to confuse is simply tid <- myThreadId
let msg = "Exception in thread: " ++ show (fromThreadId tid) ++ "\n\n" ++ exMsgBut I don't know how useful that is. |
|
Adding the thread ID is an interesting idea, it's potentially useful if one can correlate it with the eventlog or other debugging information. What about using |
|
Would we want a stack of |
Neat, I didn't know about labels. With something like: myThreadIdAndLabel :: IO String
myThreadIdAndLabel = do
tid <- myThreadId
mLabel <- threadLabel tid
`catchAny` \_ -> pure (Just "<error retrieving thread label>")
let idMsg = "id: " ++ showThreadId tid
pure $ case mLabel of
Nothing -> idMsg
Just label -> idMsg ++ ", label: " ++ labelWe can have The latter requires adding lines like
Interesting idea, though I worry about complexity. Do you have an idea for implementation? Also, I opened a draft MR to make discussion easier. |
|
I've stumbled upon one downside of the thread ids: non-determinism. 1 of the 130 affected GHC tests failed on CI since it received a different thread id (7) than my local machine (8). On the one hand, that ratio isn't bad, and I think the trade-off is worth it. But this could definitely annoy someone. A possible mitigation could be to ignore the id when the label exists. |
|
Good point about non-determinism. That suggests not adding the thread IDs to the message printed by default IMHO. #261 (comment) is a good point. It does seem that "Exception in thread with id: ThreadId 1" may be misleading if the exception occurred in a different thread and was subsequently re-thrown. What about making the thread ID or label into an exception annotation (like a very poor-resolution backtrace)? That would mean capturing it when backtraces are collected, in the thread on which it occurs, rather than in the top-level handler. (Perhaps we might worry that an exception could keep a ThreadId and hence a thread alive when it would otherwise by GCd, though.) |
|
Okay, I have been convinced that mentioning threads by default is a bad idea. The annotation idea is interesting. I presume this would involve adding a new instance to In light of that, this PR contains two changes:
The MR, then, is up-to-date. The test updates give a good idea of what it looks like. |
Intro
Hello, this is a follow-up to the accepted (unmerged gitlab MR) proposal: #231.
If you will recall, that issue adds the following exception metadata to the default handler:
On the issue tracker, @michaelpj observes that the message could be clearer still, with references to other programming languages. At the time I thought this was a good suggestion but didn't think it important enough to warrant the churn of another CLC-proposal and possible interference with the rest of the exceptions work. Now that the "callstacks are part of displayException" work has been merged, however, I think that has changed. In particular, the output with multiple stacktraces / #231 is complicated enough that I think getting other opinions would be wise.
Status quo
As of now, here is what the exception output currently looks like:
Expand example
HasCallStackonly)(the trailing newlines are due to there being a "blank" annotation that nonetheless is rendered with separating newlines.)
Current proposal
With the accepted proposal, this looks like:
Expand example
HasCallStackonly)Possible change
Taking inspiration from @michaelpj's suggestion, this could instead look like:
Expand example
HasCallStackonly)Notice that there is a new line
Exception:and the exception type info precedes the possibly backtrace info.The type info could also be more compact e.g.
ghc-internal:GHC.Internal.IO.Exception:BlockedIndefinitelyOnMVar.Implementation
Moving the exception type info before the backtrace info requires implementing the former in
SomeException'sdisplayException, not the default handler, in contrast to the accepted proposal. This seems plausible to me, as I could imagine a user wanting to override the handler (e.g. to log exceptions to a file) without losing the displayed type info.Draft MR
Thanks!
CC @tomjaguarpaw @bgamari
The text was updated successfully, but these errors were encountered: