yesodweb / wai

Haskell Web Application Interface
MIT License
834 stars 263 forks source link

Rethrow async exception #1013

Closed kazu-yamamoto closed 1 week ago

kazu-yamamoto commented 2 weeks ago

@Vlix This is the first step to fix the stuffs in #1012.

Vlix commented 1 week ago

I'm a bit busy this week, but will try to take a look next week :+1:

kazu-yamamoto commented 1 week ago

time-manager is leaked sometime:

1) In this pattern, time-manager is solely leaked:

20 QUIC dispatcher: ThreadBlocked BlockedOnSTM
24 WAI timeout manager (Reaper): ThreadBlocked BlockedOnMVar
3462 Warp HTTP/1.1 [::ffff:81.161.238.88]:53801: ThreadBlocked BlockedOnMVar
3464 Warp HTTP/1.1 [::ffff:81.161.238.88]:53803: ThreadBlocked BlockedOnMVar
3467 Warp HTTP/1.1 [::ffff:81.161.238.88]:53831: ThreadBlocked BlockedOnMVar
3570 Warp HTTP/1.1 [::ffff:81.161.238.88]:56968: ThreadBlocked BlockedOnMVar
3575 Warp HTTP/1.1 [::ffff:81.161.238.88]:57059: ThreadBlocked BlockedOnMVar

2) In this pattern, time-manager leaks other workers by holding ThreadId. I can tell this because they are in ThreadFinished. af20093 tries to fix this.

10150 WAI timeout manager (Reaper): ThreadBlocked BlockedOnMVar
10190 Warp HTTP/1.1 [::ffff:103.8.117.203]:43390: ThreadFinished
10191 Warp HTTP/1.1 [::ffff:103.8.117.203]:43392: ThreadFinished
10193 Warp HTTP/1.1 [::ffff:103.8.117.203]:43404: ThreadFinished
10202 Warp HTTP/1.1 [::ffff:103.8.117.203]:43406: ThreadFinished
10204 Warp HTTP/1.1 [::ffff:103.8.117.203]:41256: ThreadFinished
kazu-yamamoto commented 1 week ago

Both Reaper and TimeManager do not use MVar at all. Why BlockedOnMVar?

kazu-yamamoto commented 1 week ago

I hope that 1400f27 fixes the leak of TimeManager.

kazu-yamamoto commented 1 week ago

TimeManager is still leaking, sigh.

23 WAI timeout manager (Reaper): ThreadBlocked BlockedOnMVar
kazu-yamamoto commented 1 week ago

The 23 thread is gone. Why did it took so a long time?

kazu-yamamoto commented 1 week ago

OK. I understand. threadDelay is implemented with MVar. So, it can be in the BlockedOnMVar state. IOManager keeps working if connections come repeatedly. So, it can have a long life and is finished when all connections are finished. Now I think TimeManager is not leaking.

kazu-yamamoto commented 1 week ago

I'm going to merge this PR and release new versions as this PR fixes some thread leaks. Of course, comments after releasing are welcome.

Vlix commented 1 week ago

Few things I've found:

The difference between uninterruptibleMask and regular mask is only that AsyncExceptions can come through with mask in case the thread is blocked, right? Which would be when waiting for an MVar, or a threadDelay too? (I forgot how it goes with FFI calls :thinking:)

kazu-yamamoto commented 6 days ago

From the doc of Control.Exception:

If the target thread is currently making a foreign call, then the exception will not be raised (and hence throwTo will not return) until the call has completed.

That is, asynchronous exceptions cannot interrupt foreign calls.

kazu-yamamoto commented 6 days ago

ThreadIds in auto-update are not disclosed. Nobody can throw asynchronous exceptions to threads spawned by auto-update, I believe.

Vlix commented 6 days ago

ThreadIds in auto-update are not disclosed. Nobody can throw asynchronous exceptions to threads spawned by auto-update, I believe.

StackOverflow and HeapOverflow are also asynchronous, so I don't think you'd want to assume anything (though the chances are super small)

kazu-yamamoto commented 6 days ago

StackOverflowandHeapOverflow` are also asynchronous, so I don't think you'd want to assume anything (though the chances are super small)

Good point! I will think your review results again.

Vlix commented 6 days ago

OK. I understand. threadDelay is implemented with MVar. So, it can be in the BlockedOnMVar state. IOManager keeps working if connections come repeatedly. So, it can have a long life and is finished when all connections are finished. Now I think TimeManager is not leaking.

How did you figure this out, btw? I can only narrow it down to an internal built-in delay# function :thinking:

kazu-yamamoto commented 6 days ago

How did you figure this out, btw? I can only narrow it down to an internal built-in delay# function :thinking:

See: https://www.stackage.org/haddock/lts-22.42/base-4.18.2.1/src/GHC.Conc.IO.html#threadDelay

Our RTS is threaded. So, we should follow Event.threadDelay

ghc/libraries/base/GHC/Event/Thread.hs:threadDelay is defined as follows:

threadDelay :: Int -> IO ()
threadDelay usecs = mask_ $ do
  mgr <- getSystemTimerManager
  m <- newEmptyMVar
  reg <- TM.registerTimeout mgr usecs (putMVar m ())
  takeMVar m `onException` TM.unregisterTimeout mgr reg