Event-driven reaper

[DaveCTurner]

Hi,

I'd like to use the resource-pool package, but the polling model for the reaper doesn't work well with what I'm trying to do. Running the reaper every second is too frequent, but simply decreasing the polling frequency means that stale resources will hang around for too long.

I've made an event-driven reaper in the past as part of another project and it worked well before, and I've now extracted this basic functionality into the alarmclock package.

Would you consider a change to resource-pool to make it use this event-driven model rather than the current polling one? I will prepare a pull request if so.

I'm willing to believe that alarmclock is not suitable in its current form, or has some subtle bug or performance issue, so if you have comments for how it could be improved then I'd be glad to hear them.

Cheers,

[basvandijk]

An event-driven reaper sounds like a good idea. I haven't looked closely at the implementation of alarmclock yet but I noticed a potential dead-lock issue:

setAlarmVar :: MVar UTCTime -> UTCTime -> IO ()
setAlarmVar mv wakeUpTime = tryTakeMVar mv >>= \case
  Nothing          -> putMVar mv      wakeUpTime
  Just wakeUpTime' -> putMVar mv (min wakeUpTime wakeUpTime')

What happens when another thread executes and completes setAlarmVar after tryTakeMVar delivered Nothing? I think the putMVar will result in a dead-lock then.

[DaveCTurner]

The reaper thread itself is also repeatedly calling takeMVar, so isn't it OK at least until after calling destroyAlarmClock?

That jazz was to avoid a rare deadlock that took weeks to track down, but which I now think cannot occur for other reasons, so I will try and simplify it.

[DaveCTurner]

Ok, I've made that simplification to alarmclock, which led me to use a TBMQueue of length 1 rather than my own hand-rolled thing.

[DaveCTurner]

A starter for ten is at DaveCTurner@d434623

Comments welcomed. Issues I'm aware of are:

• no labelling of the reaper thread (easy enough to replace)
• no mask/restore for async exceptions (is that still necessary now no async exceptions are thrown?)
• I do not like the clunky IORef-based knot-tying used to pass the alarmclock to the LocalPools and vice versa (but lack a bright idea on how that might be fixed without more major changes).

I'm concerned that I don't fully understand the performance constraints so may have done something awful in terms of performance.

[DaveCTurner]

Now a PR.

Have been having trouble with high quiescent CPU usage in a low-usage-but-large-heap service caused by the idle GC which we've traced back to using resource-pool. Because of the large heap the idle GC was expensive, and because of the reaper thread waking up every second the idle GC was also running every second.

Switching off the idle GC cuts the quiescent CPU usage from 30% down to 0.3% which makes the difference between having to use multiple t2.large instances and a single t2.micro. This PR gives a similar improvement by only running the reaper when there's something to reap.

[winterland1989]

Is this package maintained by @bos anymore? your code looks good to me, can you contact @bos to review this?

[basvandijk]

I think I should be maintaining pool but unfortunately I don't have much time to spend on it these days. Maybe @bos can be convinced to also make @DaveCTurner a maintainer.

I did review the PR and except for the following it looks good to me:

In the alarmclock package there looks to be the possibility for a deadlock in newAlarmClock'. If an exception gets raised in the forked thread before the finaliser gets registered the joinVarwill never be put anymore and then the readMVar will deadlock. A better version would mask asynchronous exceptions before forking:

newAlarmClock' :: TimeScale t => (AlarmClock t -> t -> IO ()) -> IO (AlarmClock t)
newAlarmClock' onWakeUp = do
  joinVar <- newEmptyMVar
  ac <- atomically $ AlarmClock (readMVar joinVar) <$> newTVar AlarmNotSet <*> newTVar False
  void $ mask $ \restore -> forkIO $ restore (runAlarmClock ac (onWakeUp ac)) 
                                       `finally` putMVar joinVar ()
  return ac

An even better alternative is to use either the async or threads package.

[DaveCTurner]

Good point, I'll fix that.

[DaveCTurner]

Ok, fixed in alarmclock-0.4.0.2 by using the async package.

Also, apropos of #27, this version of alarmclock supports a system monotonic clock from the clock package as well as UTCTime. I agree that this would be preferable to using UTCTime for reasons of correctness.

[basvandijk]

@DaveCTurner thanks! Could you please bundle the commits in this PR as a single one? That makes the history a bit easier to read.

[DaveCTurner]

Sure, there you go.

[DaveCTurner]

Tidying up by closing some of my stale PRs. Sorry for the noise.