[BUG] - Leaky thread in mkTimer

[lehins]

Internal

Area

Tracing

Summary

Function mkTimer creates a thread that is never killed. Consider switching to using async

cardano-node/cardano-tracer/src/Cardano/Tracer/Handlers/Notifications/Timer.hs

Lines 32 to 37 in 9017d91

	mkTimer ioAction state callPeriodInS = do
	callPeriod <- newTVarIO callPeriodInS
	elapsedTime <- newTVarIO 0
	isRunning <- newTVarIO state
	void . forkIO $ forever $ do

Steps to reproduce
Here is an isolated example of how this thread creation in mkTimer is wrong:

#!/usr/bin/env cabal
{- cabal:
  build-depends:
    base, say
  ghc-options: -Wall -O1 -threaded
-}
{-# LANGUAGE NumericUnderscores #-}
{-# LANGUAGE OverloadedStrings #-}

module Main where

import Control.Concurrent
import Control.Monad
import Data.IORef
import GHC.Exts (fromString)
import Say

mkTimer ::
  IO ()
mkTimer = do
  nRef <- newIORef (0 :: Int)
  void . forkIO $ forever $ do
    threadDelay 1_000_000
    n <- atomicModifyIORef' nRef $ \i -> (i + 1, i)
    say $ "Forever waited for: " <> fromString (show n ++ " sec")

main :: IO ()
main = do
  tid <- forkIO mkTimer
  say $ "Started thread: " <> fromString (show tid)
  threadDelay 5_000_000
  say $ "Killing thread: " <> fromString (show tid)
  killThread tid
  say $ "Thread should be dead now: " <> fromString (show tid)
  say "Waiting for another 5 sec"
  threadDelay 5_000_000

Executing this script depicts that the thread created in mkTimer is inaccessible and will persist for the lifetime of the process, regardless if the thread that started it gets killed.

Executing this script will give you this output:

> ./timer.hs
Started thread: ThreadId 2
Forever waited for: 0 sec
Forever waited for: 1 sec
Forever waited for: 2 sec
Forever waited for: 3 sec
Killing thread: ThreadId 2
Thread should be dead now: ThreadId 2
Waiting for another 5 sec
Forever waited for: 4 sec
Forever waited for: 5 sec
Forever waited for: 6 sec
Forever waited for: 7 sec
Forever waited for: 8 sec

As you can see, killing of the thread spawned in main does not cleanup the thread that runs indeed forever

Expected behavior

Thread created in mkTimer should be cleaned up when thread that spawned it gets killed.

Another side note about Timer interface is that it unnecessarily uses TVars where IORef with atomicModifyIORef' would be a much better fit that resulted in less overhead

[mgmeier]

Executing this script depicts that the thread created in mkTimer is inaccessible and will persist for the lifetime of the process, regardless if the thread that started it gets killed.

That is the desired behaviour. The timer is supposed to run forever, even if the creating thread is killed; and the creation of Timer values is limited to within the application - so the call / spawn sites are under full control. A suggestion would be to remove the mkTimer definition from the exposed-modules and make it internal, so that it isn't accidentally misused.

Additionally, the Timer type could in theory carry a cancelTimer :: IO () action which kills the thread (or async) explicitly. But we really don't want or need that here.

I agree with the use of STM primitives being overenthusiastic here - I mean, it's not really like you want, or can, rollback your measurement of elapsed time, or having executed an IO action ;)