WIP: Ensure restarts cancel outstanding shake threads

[crtschin]

Reattempt and fixes #4985

The previous approach used a guard added to every shake thread to check whether the rule evaluated an outdated value. That approach was incomplete as the core issue is that the thread shouldn't have survived to execute.

This PR takes a different approach tracks every spawned thread and does a cancellation when a restart occurs.

[soulomoon]

The approach is to introduce a centralized management on the asyncs for the database if I understand correctly.
In this branch, we have both the decentralized AIO and the centrol cancelTrackedAsyncs. I think we should choose one instead. And we have to consider how it interacts with databaseValues.
Also trackedAsync might happens during cancelTrackedAsyncs, we might have to introduce a global lock.

Take a look at the work replacing the decentralized AIO entirely at #4927 @crtschin
Would you like to work with me on replacing the AIO entirly ?

[crtschin]

The approach is to introduce a centralized management on the asyncs for the database if I understand correctly. In this branch, we have both the decentralized AIO and the centrol cancelTrackedAsyncs.

This does introduce management of asyncs, which previously only existed in pure thunks. Though I wouldn't call AIO decentralized, AIO itself tracks the threads it spawned. The piece that this PR adds is a layer on top that ensures that any outstanding threads have all settled when doing a shake restart.

Also trackedAsync might happens during cancelTrackedAsyncs, we might have to introduce a global lock.

That should be fine as the function is recursive until it settles. So even if threads were spawned in the timespan before they died, cancelTrackedAsyncs just continues killing those children. Though this is the most contentious bit of the PR, I'm definitely open to improvements.

Take a look at the work replacing the decentralized AIO entirely at #4927

I tried looking at the PR, but there are a lot of moving bits, and is a bit hard to understand what was going on 😅. I want to avoid large-scale refactors if possible. I'll give it a second pass and try to post some probing questions.

Would you like to work with me on replacing the AIO entirly?

I don't know what you have in mind, so maybe!

[soulomoon]

I tried looking at the PR, but there are a lot of moving bits,

Yes, it contains a lot of exprimental improvements.
We should focus on replacing the AIO parts first. The idea is simple, we do not use the AIO to track the threads and kills them, we only track them in database and use things like cancelTrackedAsyncs.

That should be fine as the function is recursive until it settles. So even if threads were spawned in the timespan before they died, cancelTrackedAsyncs just continues killing those children.

As we discuss in #4988, leaking AIO thread is the problem, so a lingering thread won't be killed by any parent. So it is still possible to escape from interleaving trackedAsync and cancelTrackedAsyncs .

[soulomoon]

Perhaps we should guard the databaseAsyncs here, so no new async would be created in between readTVar databaseAsyncs and killing them. Then we can ensure no one leak ?

[soulomoon]

I got an idea to fix the AIO leaking bug @crtschin, add a switch for AIO, when the thread that have the scope exit, we flip the switch that would halt any future registration. Then we won't have lingering threads that won't be killed by a session restart.

[crtschin]

add a switch for AIO, when the thread that have the scope exit, we flip the switch that would halt any future registration.

Trying to understand what you mean here. So adding a guard to trackedAsync so an atomic increment of the database step in STM and thread being spawned can't do so? I think that works and avoids the need for the loop in cancelTrackedAsyncs.

[soulomoon]

I mean attacking the bug directly #4988 (comment), then we do not need the extra bookeeping of threads.

more presicely, close the possiblility to register a new async in asyncWithCleanUp once the corresponding cleanupAsync is invoked.
e.g. change AIO to newtype AIO a = AIO { unAIO :: ReaderT (IORef (Maybe [Async ()])) IO a }, cleanupAsync change the IORef content to Nothing, then the corresponding asyncWithCleanUp won't be able to register.

[crtschin]

I see, so avoiding threads from being spawned in the first place when a restart occurs, instead of trying to track/find and kill them.

[crtschin]

Superceded by #5027