httpServe appears to be interfering with slave-thread

[bitemyapp]

Children aren't getting harvested on ctrl-c in GHCi, can see their output continue even though the web server itself successfully shut down.

I think slave-thread needs 1 a ThreadKilled 2 exception to bubble up to the parent thread (the one in main which invoked httpServe) in order for it to do its thing.

This looks like a good candidate for starting to narrow things down.

I'll try to put together a minimal repro in a bit.

Some context from IRC:

< mightybyte> One of our apps started exhibiting this behavior a few months back.
< bitemyapp> I think perhaps it is not letting the ThreadKilled bubble up.
< mightybyte> And I think it happened around the time that some code was added that probably forked some threads.
< mightybyte> So that all lines up with what you're saying.

[bitemyapp]

Repro project: https://github.com/bitemyapp/snap-slave-thread-repro/blob/master/src/Main.hs#L34-L55 (lts-2.14)

Screenshot:

[bitemyapp]

Also :main vs. main makes no difference.

[gregorycollins]

What's the desired behaviour? You want httpServe to rethrow ThreadKilled?

We can do that if necessary, but the repro example doesn't seem kosher to me. Up to this point the idea has been that the server loops forever until it gets ThreadKilled from outside, and then it exits. Rethrowing it seemed superfluous, since the only reason httpServe would exit is if it got a ThreadKilled.

Looking at the example:

do
    ...
    SlaveThread.fork canary
    _ <- try $ httpServe conf site :: IO (Either SomeException ())
    cleanup

I think the only reason you would get the ctrl-c semantics you want in normal operation is that the main thread is special and uncaught exceptions from there kill the whole program immediately. And really immediately -- child threads don't get gracefully killed in this scenario.

Anyways it should probably look like this:

do
    ...
    bracket (SlaveThread.fork canary) killThread $ const $ do
        void $ try $ httpServe conf site :: IO (Either SomeException ())
        cleanup   -- cleanup should be bracketed here too, btw!

I think if you write it in this style the issue wouldn't occur.

[mightybyte]

Greg's bracket solution seems to work for me.

[gregorycollins]

...and if you want to do it really really correctly, you should wait for the child thread to finish processing before you exit from main --- these days I would use the async package for this, your bracket cleanup task turns into \x -> cancel x >> wait x in that scenario.

[bitemyapp]

@gregorycollins I would expect httpServe to rethrow ThreadKilled so that any cleanup/shutdown not strictly part of Snap can get the appropriate notice to do what it needs to do. I don't know why I would want my web server to hide SIGINT or similar signals.

This reproduction is not a perfect simulation of what we're doing in our application.

In our application, it's doing a SlaveThread.fork from the App init.

app :: SnapletInit App App
app = makeSnaplet "Server" "Server" Nothing $ do
...
    spawnAgent frequencyInMinutes f = liftIO $ SlaveThread.fork $ forever
        (threadDelay (frequencyInMinutes * 1000000 * 60) >>
            logExceptions f)

Snap preventing slave-thread from being able to do its thing when we're forking from within the httpServe invocation seems even worse than breaking the separately forked canary even though that should work fine as well.

Your suggested edit to the example doesn't fix the problem and isn't adaptable to the "real code". I could modify the example to be more like the real thing, but I don't think that'll help. I don't know the threadId of everything I forked via SlaveThread at the top-level where I fire off httpServe. The whole point of SlaveThread is, "when I die, kill my children". If you don't let the parent thread die, nobody that uses slave-thread can use Snap. Much of the point of slave-thread is to avoid using async for this particular problem.

[bitemyapp]

@gregorycollins @mightybyte Would you be willing to make it stop hiding ThreadKilled? Hiding ThreadKilled is like catching and gobbling OutOfMemoryError on the JVM.

[gregorycollins]

@bitemyapp I don't agree that ThreadKilled is in any way like OutOfMemoryError. ThreadKilled is a signal that the thread should clean itself up and exit, and that's what httpServe does. Semantically httpServe is bottom, i.e. it will never return unless it receives an async thread, so it "owns" the thread now and getting ThreadKilled forces it to exit. To me it's not cut and dried that rethrowing is preferable here, and changing this might break clients if they depend on the existing behavior.

The whole point of SlaveThread is, "when I die, kill my children".

I think you're misunderstanding the semantics here, read the implementation: if you call SlaveThread.fork from a normal thread, you don't get cleanup of children on exit -- and think about it, if you did, we wouldn't be having this discussion because your child threads would have killed themselves. That behaviour only happens for threads themselves forked by SlaveThread.fork (which makes sense, the forkIO'd thread has to be wrapped by the slave-thread cleanup routing).

Besides, like I said before, I don't think your finally handlers get a chance to run in the event of main-thread seppuku, i.e. you've been relying on unclean shutdown. A couple of questions before we consider changing this default:

• does this work for you if you run the snap server from a forked slave-thread? You should get the slave-thread cleanup for the subthreads in this case.
• in the short term why not try:

httpServe foo `finally` throwIO ThreadKilled

[bitemyapp]

@gregorycollins I did try the latter, didn't fix it. I'm left with a few possibilities.

1. GHCi is special (I don't think that applies here, but I could be wrong)
2. The way Snap manages the thread-pool is causing slave-thread not to clean up after itself when the web server dies (the forks are from within the app init)

So, I think you're right, if my rethrow didn't fix it – changing it won't fix it either. Do you have any ideas as to why a SlaveThread.fork from App init wouldn't clean up when the server gets ctrl-c?

[gregorycollins]

I mean, I wouldn't expect for those slave threads to be cleaned up, not unless

1. the thread that forked them was itself run by SlaveThread.fork, or
2. you explicitly killed them yourself using killThread in a finally or bracket block

[gregorycollins]

Also a potential reason why the behaviour is different in ghci: you don't get the main-thread-seppuku "feature" there because the ghci interpreter thread is not the main thread.

[bitemyapp]

@gregorycollins GHCi does still show the ThreadKilled exception, seems like GHCi shouldn't break that behavior but I don't know what it really does in this case.

I'm pretty sure your first item is impossible because it doesn't seem like Snap provides a way to control or configure thread pool management.

Second item is ~roughly what I was thinking I would have to do, but I don't have the ability to do this from outside httpServe without registering the ThreadIds somewhere.

[gregorycollins]

Running the snap initializer in a slave-thread itself would mean that threads it directly forked (using SlaveThread.fork) would get cleaned up on exit. Of course, that just means that you'd have to write a latch in the main thread to wait for clean shutdown on ctrl-c there.

If the bracket idiom is too much of a PITA you can try ResourceT.

[bitemyapp]

@gregorycollins I might try SlaveThread forking the httpServe and bracketing on an async value or something so I know when it ended.

[bitemyapp]

@gregorycollins you kicking this around with me is helping clear up my confusion on this, thank you very much!