Repository navigation
Fix console deadlock on Unix by locking Console.Out first (#519) - #536
Merged
Merged
Conversation
On Unix, .NET ends every console stream write in lock (Console.Out). After init, Console.Out is Expecto's synchronized writer, so a thread in Console.Write holds that lock and then waits for the ANSIOutputWriter buffer, while a thread flushing the buffer waits for Console.Out. A printfn in a test racing a write to Console.Error or Expecto's own logger could hang the test run. ANSIOutputWriter now takes Console.Out before buffer when it prints or flushes. The progress spinner takes it before isRunning, because the flush path holds Console.Out when it enters the FlushStart handler, which locks isRunning.
nojaf
added a commit
to ionide/FsAutoComplete
that referenced
this pull request
Oct 9, 2026
…s it found (#1554) Local runs of the LSP test suite crashed, hung, or needed about 11 GB, and CI split the suite into four shard processes to make it fit. This PR makes the suite run reliably in one process per target framework, locally and in CI, and fixes the server defects the investigation turned up. ## Server fixes (`src/`) - **A project load that throws no longer ends the server.** Before, the exception escaped from two background starts (`asyncaval.AddCallback` and the project reload debounce) and the runtime ended the process: in an editor, the language server died. Now it is logged, and requests return errors as before. Tests: `AdaptiveExtensionsTests.fs`, `WorkspaceLoadFailureTests.fs`. Each one ends the test host without its fix. - **A disposed server can be collected.** Two references kept every server alive: each `AdaptiveState` wrapped the process-wide FCS `FileSystem` and never unwrapped it, and the `DiagnosticCollection` agents kept their sender after `Dispose` (the state never disposed the collection either). Now one shared file system asks the open files of each live state, newest first (the same lookup order as before), and a state unregisters on `Dispose`. - **A disposed server stops the analysis it started.** The `fileChecked` handler is cancelled on `Dispose` and logs its errors. Before, `documentAnalyzed` could reach the client after `Dispose`, and an exception there ended the process. - **`vstest.console` stops after each test discovery and run.** Before, each discovery or run left one running until FSAC exited. - `ProcessHelper.WaitForExitAsync` uses `TrySetCanceled`. `SetCanceled` threw inside `Cancel()` when the process had already exited. The `dotnet new --list` process is now disposed. - Fantomas.Client 0.12.0-beta-004: `Dispose` now ends its mailbox loop (fsprojects/fantomas#3530). Ionide.ProjInfo 0.75.1: loaders in one process share one lock around MSBuild's `BuildManager` (ionide/proj-info#184). Before, two servers that loaded at once failed with "a build is already in progress". ## Test suite - **SDK check.** When `global.json` resolves an SDK whose major version differs from the test host's, the run is one failing test whose message is the `dotnet new globaljson` command that pins a matching SDK. Before, MSBuild 11 was loaded into a net10 or net8 host, and the process aborted. The pin is gitignored, and `build.fsx` writes the same file in CI. - **No more hangs at exit or in the console.** `main` ends with `exit`. Expecto 11.1.1 fixes a lock-order deadlock between its console redirection and .NET's console lock on Linux and macOS, hit when a log line and a test's `printfn` overlapped (haf/expecto#519, haf/expecto#536). - **Expecto 11.1.1.** YoloDev.Expecto.TestSdk moves to 0.16.1, the first adapter that accepts Expecto 11, and brings Microsoft.Testing.Platform 2.3.3. `dotnet test` still runs through VSTest. Expecto.Diff follows Expecto. - **Parallel groups in one process.** The LSP test groups run four at a time (`--parallel-workers`). The groups that change process-wide state (GoTo, Templates, Test Explorer) run one after another at the end. Each group disposes its servers when its last test finishes, and `ServerHandle` (a `DispatchProxy`) stands in for the server in cached test closures, so the server can be collected. - **Each project restored once.** Each test project directory is cleaned and restored once per run, instead of once for every server setup. A directory is cleaned and restored again when `DotnetCli.build` built into it. - **Waits on server events instead of fixed sleeps** in `changeTextTo`, `saveText`, `parseProject`, `EmptyFileTests` and `ScriptTests`. The waits for server notifications follow `FSAC_TEST_DEFAULT_TIMEOUT`, as the per-test timeout does (120 s in CI, 10 s locally). - **Per-test timeouts that stop the test.** Expecto's `Test.timeout` reports a timeout, but the test keeps running next to the tests after it. `Helpers.Expecto.cancelOnTimeout` cancels an asynchronous test and gives it 10 s to stop. Every LSP test gets it, also the tests that had no timeout. Tests: `TimeoutTests.fs`. - **Run timeout fails the run.** When the executable is cancelled after `TEST_TIMEOUT_MINUTES`, it now exits with a failure. Expecto returned 0 for a cancelled run. - **Temporary directories.** They are in `TestResults/tmp/<pid>`, so every `dotnet` started there uses the pinned SDK, and stub `Directory.Build.props/.targets` files keep the repository's build files out. Directories left by processes that ended are deleted at startup. - `DotnetCli.build` runs in the project directory, shows stdout when a build fails, and stops a build that takes longer than 5 minutes. - The run ends with the list of tests that did not pass, not every test. - The snapshot test "Adding nuget package should cause project load" expects at least one reload, not exactly one: on macOS, FSEvents reports the project file and the assets file separately. - AGENTS.md: a "Running Tests" section (build one framework, run the executable with a filter, the SDK pin, one compiler while prototyping, the timeouts). - **Removed test:** "shard one starts in a separate untitled document range" (`Utils/Server.Tests.fs`), together with sharding. ## CI - `test-ci:<tfm>`: one `dotnet test` per framework, with `Expecto.parallel-workers=4`, instead of four shard processes and merged coverage reports. Nothing read those reports. - Packages are restored once before the build, and the build is run with `PaketDisableGlobalRestore=true`. Paket.Restore.targets started a `paket restore` for every project, and on macOS one of these was sometimes killed (exit 137). - .NET 8 is installed on every leg, because the Test Explorer sample projects target net8.0. - Actions are pinned by commit SHA, and Dependabot updates them every month. `release.yml` moves to `changelog-reader-action` v2.4.0, which renames its output `log_entry` to `changes` and takes the version without the `v` of the tag. ## Results | | `main` | this PR | |---|---|---| | Local full run, net10.0, one compiler | crash or hang | 3,217 passed, about 90 s | | Peak memory, one process, whole suite | about 11 GB | 4 to 5 GB | | CI test step, average (min to max), Linux | 290 s (214 to 318) | 259 s (204 to 283) | | CI test step, Windows | 312 s (226 to 387) | 255 s (212 to 365) | | CI test step, macOS | 677 s (393 to 941) | 525 s (453 to 609) | CI: one green run of each, [main](https://github.com/ionide/FsAutoComplete/actions/runs/37750707258) and [this PR](https://github.com/ionide/FsAutoComplete/actions/runs/37933293253). ## Not in this PR - After a failed load, the server never loads again until it is restarted: the failure is cached, and project files are only watched after a successful load. - After the analyzers are loaded, they do not run for files that are already open. Because of this, `ExtensionsTests` keeps one 5 s sleep. - Synchronous tests still use Expecto's `Test.timeout`, which cannot stop them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Unix, .NET ends every console stream write in lock (Console.Out). After init, Console.Out is Expecto's synchronized writer, so a thread in Console.Write holds that lock and then waits for the ANSIOutputWriter buffer, while a thread flushing the buffer waits for Console.Out. A printfn in a test racing a write to Console.Error or Expecto's own logger could hang the test run.
ANSIOutputWriter now takes Console.Out before buffer when it prints or flushes. The progress spinner takes it before isRunning, because the flush path holds Console.Out when it enters the FlushStart handler, which locks isRunning.
Fixes #519