Guard against leaking threads while restarting shake

[crtschin]

Alternative variant of #5021.

Third time's the charm.

Instead of keeping track and killing threads via the shake database, do this locally in the AIO abstraction that does cleanup. Once shutdown, the AIO is closed, so threads that are in the process of being spawned get killed instead.

[soulomoon]

should only run on succ registeration instead of finding out the scope have ended and killing it ?

[soulomoon]

e.g. async thread should know if itself is registered, it only run the body if it is registered or do nothing otherwise.

[soulomoon]

waitConcurrently_ should apply the same run the body only if registered pattern.
And we should not spread the throw everywhere and trigger the expection handler multiple times. I've made some ajustments. @crtschin feel free to revert any unsensible changes.

[soulomoon]

I am seeing some unintended failure in the benchmark result, there must be something I am missing.

[crtschin]

waitConcurrently_ should apply the same run the body only if registered pattern.

For individual threads I understand, as it's arbitrary side-effecting user code, but why for waitConcurrently_ as well? Is there an error condition you see here?

I've kept a variant of your changes in the PR, but I don't understand the need yet.

And we should not spread the throw everywhere and trigger the ..exception handler multiple times.

Exception handlers are quite cheap in Haskell. If there's a broken precondition and a thread needs to halt, it's more efficient to have the thread immediately die on its own than wait for a different thread to kill it, which can also be quite tricky if that supervisor thread already died.

I've made some adjustments.

I gave this a review, and pushed some small edits.

[crtschin]

IIRC, calling error makes GHC throw an IOError, which is less easy to work with than AsyncCancelled which is specifically intended to indicate that a async green thread didn't finish the work it intended.

[soulomoon]

perhaps just use undefined ? Since we should never inspect the result after the gate shows that it leave its' scope. And the thread should simply do nothing then finish if it knows it is out of scope.

[crtschin]

undefined is the same as error in this case. AsyncCancelled still makes more sense to me here. The thread hit a invariant, so should terminate (safely). Things we definitely want to avoid are hangs, and exceptions that would otherwise cause HLS to crash hard, which things like "undefined" or "error" open the app up to.

[soulomoon]

If it hangs or exceptions are invoked, we must be doing something wrong in our model, a hard crash is what we wanted so we can fix them.

[crtschin]

I believe there's a logical gap here now. If the AIO is interrupted between these lines, the async spawned isn't killed.

Notice that runAsyncIfRegistered has a check, and registerAsyncs has a subsequent use. If cleanup fires after the runAsync* call, the lock passes and spawns the async, but the registration fails because the scope is closed, orphaning the thread.

[soulomoon]

If cleanup fires after the runAsync* call, doesn't it mean we would cleanup the async during cleanup ?

[crtschin]

Yes, I meant if we didn't cancel these unregistered threads that land in between closing the scope in the cleanup, and the registration.

[soulomoon]

we should only create two types of threads,
the ones that are registered and canceled by cleanup.
the others that are not registered, should ended itself without doing anything.

we can choose not to kill the second ones, only let it finish.

[soulomoon]

waitConcurrently_ should apply the same run the body only if registered pattern.

For individual threads I understand, as it's arbitrary side-effecting user code, but why for waitConcurrently_ as well? Is there an error condition you see here?

Notice we register them into the AIO later. So they are AIO async too and should be managed by AIO.

[soulomoon]

necessary invariant:
Only registered threads execute force or rule bodies.
Registered threads are cancelled exclusively by cleanupAsync.
Rejected threads perform no work and terminate themselves.

[crtschin]

I tweaked the abstractions a bit over the last few weeks.

• Made a new Scope type that encapsulates the "scope" threads are spawned in via AIO.
• The above uses a lock instead of IORef to lock registration instead of trying to fix it adhoc using atomic IORef interactions.
• Make it possible to distinguish between a thread close due to an exception vs the scope being closed.
• It'll now dirty the key automatically if the scope is closed while the key was being processed.

I think the current implementation matches your invariants as well.

[soulomoon]

Bug reproduction

The test rapid edits then save does not strand a stale diagnostic was run 1,000 times on each exact implementation:

Commit	Implementation diff	Passed	Failed	Result
b7e5d2526	Baseline	993	7	Bug reproduced
5a250886c	View diff	1,000	0	No reproduction
48f297100	View diff	1,000	0	No reproduction

Both fix commits prevent the behavior exercised by this regression test.

b7e5d2526 and 48f297100 were tested with only the unchanged regression test added; their implementation code was otherwise unchanged.

Benchmark comparison

Commit b7e5d2526 is the benchmark baseline.

• 5a250886c (diff, benchmark run):

  • Generally close to baseline.
  • code actions after cradle edit is approximately 2.3–2.6× slower:
    • GHC 9.12: +126.6%
    • GHC 9.14: +164.3%
  • Overall geometric-mean totalT change: +1.02%

• 48f297100 (diff, benchmark run):

  • Remains close to baseline and avoids the cradle-edit regression:
    • GHC 9.12: +2.5%
    • GHC 9.14: −0.9%
  • Overall geometric-mean totalT change: −0.57%

Excluding the cradle-edit outlier, the two fix commits are effectively tied. Overall, both fix the tested bug, but 48f297100 has the better benchmark result.

---------above comparison generated by codex -------

@crtschin The newest head introduce some slowdown, I think it is due to the extra complexity from database repairing and a few places which blurred the ownership model, while the old method stayed minimalized and avoid performance regression.

Do you mind use View diff instead.

Also I think we should shape the test rapid edits then save does not strand a stale diagnostic better, so we can have a more deterministic failure result.

[crtschin]

Thanks for the benchmark run @soulomoon!, I removed an optimization while I was refactoring for the sake of uniformity (the setup runs singular actions inline, instead of separately spawning a thread). I re-added it and now get similar performance to the baseline again now, when running the benchmark locally.

Do you mind use View diff instead.

I'm hesitant, the main difference is in how a thread who's scope is closed, terminates. The branch does a loop expecting the thread that initialized the scope to land the cancel cleanly. This is cumbersome and tricky, as nothing inherently guarantees that. The risk here being zombie threads that are orphaned and endlessly take up resources.

[soulomoon]

if there is an exception that is not caused by session restart, it should be a bug for the hls-graph, we should not repair it. see defineEarlyCutoff' and actionCatch, there we catch all the rule's errors and leave only the isAsyncException to surface to the hls-graph to handle.

[soulomoon]

That means if build does not compelete, it must be a session restart, we should not retry it.

[soulomoon]

The risk here being zombie threads that are orphaned and endlessly take up resources.

If there are zombie threads, there must be some other bugs hidden in other part of the codebase which we should also rule out. It is better to do just enough to reveal the problem than over do it and cover an unstable problem up.

[crtschin]

there must be some other bugs hidden in other part of the codebase which we should also rule out.

Proving the absence of bugs isn't possible.

It is better to do just enough to reveal the problem than over do it and cover an unstable problem up.

I don't understand how having a thread hang is a better solution than having the thread throw. The throw is more immediate and visible (and reportable by a user). The hang is the one that invisibly worsens the state (where the main symptom would be increased memory usage and performance degradation).

[soulomoon]

I don't understand how having a thread hang is a better solution than having the thread throw. The throw is more immediate and visible (and reportable by a user). The hang is the one that invisibly worsens the state (where the main symptom would be increased memory usage and performance degradation).

My point is not about a hang is better than a throw, I am sorry for giving you that impression. What I am trying to say is that to be able to observe the fact that the scope miss the kills, the parent should not throw immediatly once it sees a failed spawning due to scope closed since it cover up the problems(We do not know wether the scope kill is just late or won't happen). I agree it is not very ideal to let it hang, perhaps a better way is, wait a bit for the scope kills and if that did not happen, log something out and throw.

[soulomoon]

Here is the newest perf comparison after the sync to the newest master
I realized can further optimize away the asyncWithCleanUp. I opened pr #5039 to integrate that part. Then we do not have to fix the asyncwithcleanup for the scope issue. And focus on waitConcurrently_

The old fix approach spawnning remain lockless, but the new one use a MVar as a lock to wait for the spawn, might be a bit slower. But usualy only one child for the same scope would be spawned in the same time, so perf degradation might not be obivious.

Updated benchmark comparison

Both implementations are based on the same master commit, 16bf046629.

Exact head	Diff	Geometric-mean totalT vs master	Summed totalT
3108a99827	View diff	−0.53%	−1.04%
480fcf8bcc	View diff	−4.83%	−4.42%

Across 83 common successful benchmark cases, 480fcf8bcc is:

• 4.21% faster geometrically
• 2.83% faster by median
• Faster in 57 of 83 cases

code actions after cradle edit

Configuration	3108a99827	480fcf8bcc
GHC 9.12	+1.71%	−6.36%
GHC 9.14	−1.25%	−5.79%

Benchmark runs:

• 3108a99827 benchmark
• 480fcf8bcc benchmark

Conclusion: both implementations are slightly faster than master, but 480fcf8bcc has the better overall performance.