Fix leak in H2 manager

[edsko]

See ManagedThreads.

Closes #154.

[edsko]

I know this change doesn't do anything, it's just for clarity.

[edsko]

This is the critical line.

[kazu-yamamoto]

I'm confused.
If this is critical, Data.Map.Strict is good enough.

[edsko]

No, that is not correct. Without the $!, it doesn't matter which function we use, because it is never called! It will just be a thunk build up of insert .. delete .. insert .. delete .. insert ... That's why it's critical to force the map to WHNF (not NF), so that we actually call the function we use.

[kazu-yamamoto]

We are using Strict and StrictData but threads' is in a tuple.
That's why?

[kazu-yamamoto]

The improve-manager branch now use modifyTVar'.

[edsko]

No, that is not the reason. Strict does not make function application strict, it only makes bindings strict. So if we have a module in which Strict is not enabled (standing in for writeTVar)

module A where

ignore :: a -> String
ignore _ = "Hi"

then in

{-# LANGUAGE Strict, StrictData #-}

module B where

import A

ex1, ex2, ex3 :: IO ()
ex1 = putStrLn (ignore undefined)
ex2 = putStrLn (ignore $! undefined)
ex3 = let x = undefined in putStrLn (ignore x)

ex1 prints "Hi" whereas ex2 and ex3 both throw an exception.

[kazu-yamamoto]

I'm not completely sure but I guess that the true source of this leak is ThreadId.
If so, we should use Weak ThreadId instead.
See: https://kazu-yamamoto.hatenablog.jp/entry/2024/11/20/160218

[kazu-yamamoto]

Uhhm. Weak is not an instance of Eq and Ord.
So, we cannot use Map, sigh.

[kazu-yamamoto]

We can convert ThreadId into Int via Show.
Then we can use IntSet (Weak ThreadId, TimeoutHandle).

[kazu-yamamoto]

I guess that https://github.com/kazu-yamamoto/http2/tree/improve-manager fixes this thread leak.

[edsko]

I guess that https://github.com/kazu-yamamoto/http2/tree/improve-manager fixes this thread leak.

I will try and let you know.

[edsko]

I can confirm that with 5.3.8 the memory leak is gone. I am seeing a lot of uncaught TimeoutThread exceptions in my tests, significantly more than before, but will try to work around that our side.

[kazu-yamamoto]

Please don't work around it your side.
TimeoutThread should be caught in the API of manager.
We should fix the API.

BTW, I have moved the thread manager from http2 to time-manager.
I will fix your issues in time-manager.

https://github.com/kazu-yamamoto/wai/blob/check-async-exception/time-manager/System/ThreadManager.hs

[kazu-yamamoto]

I see that the exception is leaking in http3 with the time-manager version.
I will try to understand why.

[kazu-yamamoto]

It's not leaking.
Just caught in the action passed to forkManagedTimeout.
I should follow the rule where asynchronous exceptions of others should be re-thrown.

[edsko]

Ok, let me know if you have something that you'd like me to try out. Thanks for looking into this!

[kazu-yamamoto]

My bad.
withHandle does not catch TimeoutThread since it returns IO a.
To catch it, the return type should be IO (Maybe a) where Nothing is returned in IO monad when timeout is fired.

I'm now considering whether or not this breaking change is acceptable.

[kazu-yamamoto]

@edsko I have released several packages hoping that your problems are all gone!

[edsko]

Haha, all my problems gone eh? 😬 Now that would be a nice Sinterklaas gift 😁 I will try it out and let you know! :-)

[edsko]

@kazu-yamamoto , my man , I do believe you are right! All the grapesy tests pass, including our stress tests (which check for memory leaks) with http2-5.3.9, http2-tls-0.4.5, network-run-0.4.3 and time-manager-0.2, with no workarounds required on our side. I just had to patch time-manager to export the KilledByThreadManager exception (yesodweb/wai#1016). I'm actually on holidays as of tomorrow (for 1.5 weeks), this is a great start to my time off :) Thank you! :)