Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add a crash handler at initialization #48

Open
wants to merge 1 commit into
base: postgres
Choose a base branch
from
Open

add a crash handler at initialization #48

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Application.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import Network.Wai.Middleware.RequestLogger (Destination (Logger),
mkRequestLogger, outputFormat)
import System.Log.FastLogger (defaultBufSize, newStdoutLoggerSet,
toLogStr)
import GHC.Conc.Sync (setUncaughtExceptionHandler,
getUncaughtExceptionHandler)


-- Import all relevant handler modules here.
-- Don't forget to add new modules to your cabal file!
Expand All @@ -38,6 +41,13 @@ mkYesodDispatch "App" resourcesApp
-- migrations handled by Yesod.
makeFoundation :: AppSettings -> IO App
makeFoundation appSettings = do
-- A relatively unkown GHC feature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unkown ==> unknown, though I think it would be better to leave this line out entirely, it doesn't add anything to the user's understanding of what's going on.

-- This can be particularly useful for reporting initialization errors
origHandler <- getUncaughtExceptionHandler
setUncaughtExceptionHandler $ \ex -> do
putStrLn $ "[crash] " <> tshow ex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least to me, crash implies that the program crashed, not a single thread. I know it's verbose, but I would probably go with something like "Thread died with exception: ", though I don't feel that strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[crash] is mean to be a log section, so it should be small. This is in fact used for when the program crashes: it only catches exceptions on the main thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using setUncaughtExceptionHandler will affect child threads too, e.g.:

import Control.Concurrent
import GHC.Conc.Sync

main :: IO ()
main = do
    setUncaughtExceptionHandler $ \ex -> print ("uncaught", ex)
    forkIO $ error "child"
    threadDelay 1000000
    error "main"

If the goal is to attach an exception handler to the main thread, I think wrapping up the call to appMain with a catch should be sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess warp is catching the other exceptions. One of the main goals is to catch any initialization errors. But catching errors from user created threads would be very useful also. So I can change the logging to just indicate that a thread crashed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's initialization exceptions, I don't see why using catch around the entire appMain is insufficient. It's much better to use something like that instead of modifying a global exception handler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to know about any thread exception that is not caught. I think the global exception handler is safe in that it is only for an application author and this is application code. It would be better to prove it by only placing this at the beginning of the main function, but this should also be available for development with GHCi or yesod evel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just addressing the comments you made here: that the exception handler is for the main thread, not the child threads. If you are trying to address child threads, I'd just like to be clear about what the goals are. Also, this is the first I'm hearing in this discussion about GHCi or yesod devel. So I think it's worth starting over with a simple question:

What exactly is your goal in writing this exception handler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reporting all uncaught exceptions (in production and in development) seems like a good goal. A crash during initialization is a subset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then I think my original comments apply:

  • crash is a bad term for an individual child thread dying.
  • For initialization errors, it would be better to use catch explicitly.

That said, I'm not convinced that this changed default is a good thing. Any threads forked by Warp should never die with exceptions (since Warp should catch those exceptions itself and use its own exception handler), and for threads forked in other ways: do we really know better than GHC what the default should be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your bullet points.

GHC takes a diplomatic stance because only the application author can know what to do: I sends errors to rollbar.com.

origHandler ex

-- Some basic initializations: HTTP connection manager, logger, and static
-- subsite.
appHttpManager <- newManager
Expand Down