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.

[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.

[crtschin]

How about this? The primary issue is that we capture the scope and store it in the database. Instead of capturing scopes, each stored key opens its own scope and descends from the build that forced it.

I've committed this to the branch.

[soulomoon]

how does it make sure all the threads would be killed on restart ?

[crtschin]

So instead of the withRunInIO call capturing the AIO scope and putting that scope in the database, we (recursively) capture and cancel the running threads when stored thunks are forced.

Each thunk value, when forced, spawns a fresh scope capturing all threads it also spawns through forcing other thunks. When any thread receives a cancel (either through restart, or from a parent forcing thread), it cancels its descendants through the scope that's created when it is forced.

[soulomoon]

It look like a good idea to me, just need to make sure no perf regression and the bug was fixed.
If I understand correctly, now the thread that does the registration and the cleanup would be the same thread so that race would no longer be possible.

[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.

[crtschin]

@soulomoon I like the simplification in #5039! I'll wait with continuing with this PR until that ones resolved. I think I can drop some code here once that lands.

[soulomoon]

ready to push it forward ? @crtschin

[soulomoon]

I believe it is a very elegant solution.

If I understand correctly, now the thread that does the registration and the cleanup would be the same thread so that race would no longer be possible, we should enhence the note to address that, also can you generate a report like this one #5027 (comment) (very easy asking an ai to do this mundane job), so that we can confirm the bug is fixed and no perf regression. And then dogfood it a bit. After that I think it is good to merge.

[crtschin]

now the thread that does the registration and the cleanup would be the same thread so that race would no longer be possible, we should enhance the note to address that

Done 👍🏼

also can you generate a report like this one #5027 (comment) (very easy asking an ai to do this mundane job), so that we can confirm the bug is fixed and no perf regression.

Had Claude do this, it reported the conclusion that it's slightly faster, though I think this is probably just benchmarking noise from the VMs.

Claude output

Claude Output:

## Bug reproduction

I used the deterministic hls-graph test from this PR, `Rule computations own their scope / cancels the deps of a stale key with the build that forces it`, and ran it 1,000 times on each side (GHC 9.12.3):

| `Database.hs` from | Test | Passed | Failed | Result |
|---|---|---:|---:|---|
| master (`187fcd4a6`) | this PR | 0 | **1,000** | Bug reproduced |
| this PR (`c50271009`) | this PR | **1,000** | 0 | No reproduction |

On master, every run fails with a 2 s timeout (`expected: Just (), but got: Nothing`). The full hls-graph suite passes on this PR.

## Benchmark comparison

[Benchmark run](https://github.com/haskell/haskell-language-server/actions/runs/36627177012) for `c4af5a290`, compared with master `187fcd4a6`. There are 83 cases that passed on both sides (cabal and lsp-types, GHC 9.12 and 9.14):

- Geometric-mean `totalT`: **−1.39%**
- Summed `totalT`: −1.49%
- Median: −0.44%
- Faster in 51 of 83 cases
- Geometric-mean `maxResidency`: −1.52%

`code actions after cradle edit` (the case that regressed before):

| Example | GHC 9.12 | GHC 9.14 |
|---|---:|---:|
| lsp-types | +0.1% | −0.8% |
| cabal | failed on both sides | failed on both sides |

The largest slowdowns are on cabal 9.12 cases that take 1 to 2 s: `hover` +18%, `completions` +14%, `code actions` +12%, `getDefinition` +11%. That is at most 0.29 s. The same cases on lsp-types 9.12 are within ±7%. I think this is runner noise from one run.

Conclusion: the bug is fixed, and this PR is slightly faster than master. Next I will dogfood it.

And then dogfood it a bit.

I've used it in a few small editing/code-reading and I haven't seen the buggy behavior again.

[soulomoon]

Good work, look good to me

[crtschin]

Thanks for sticking with me on this issue @soulomoon, I wholeheartedly appreciate the rigor!