Remove FreeLibrary invocations in managed SOS host
#5586
Merged
+3
−2
Conversation
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
hoyosjs
previously approved these changes
Oct 22, 2025
noahfalk
reviewed
Oct 23, 2025
| } | ||
| if (_dacHandle != IntPtr.Zero) | ||
| { | ||
| DataTarget.PlatformFunctions.FreeLibrary(_dacHandle); |
Member
There was a problem hiding this comment.
Do we still want to use cDac with older versions of SOS that won't have this change?
cDAC hosted under NativeAOT could call LoadLibrary() on itself to bump the reference count up. That should prevent the OS from trying to unload it regardless whether SOS calls FreeLibrary().
Member
Author
There was a problem hiding this comment.
Do we still want to use cDac with older versions of SOS that won't have this change?
I think this will be acceptable. It will keep SOS open to a race condition, but it using the cDAC isn't strictly supported today.
cDAC hosted under NativeAOT could call LoadLibrary() on itself to bump the reference count up. That should prevent the OS from trying to unload it regardless whether SOS calls FreeLibrary().
This would work, but I think avoids the issue we will need to face eventually. Merging this after the 10 fork would allow us to find any potential issues and fix them before 11.
steveisok
approved these changes
Dec 4, 2025
Member
steveisok
left a comment
steveisok
left a comment
There was a problem hiding this comment.
I think we should push this forward and if we find problems, we can look to incorporate the ref counting approach or other workarounds.
This was referenced Dec 5, 2025
max-charlamb
added a commit
to max-charlamb/diagnostics
that referenced
this pull request
Dec 5, 2025
commit 5de3340 Author: Max Charlamb <[email protected]> Date: Fri Dec 5 11:36:00 2025 -0500 move native SOS test helpers to test folder (dotnet#5652) * Moves `runcommand` and `DesktopClrHost` from under the SOS package to `src/tests/` commit d1421e4 Author: Max Charlamb <[email protected]> Date: Fri Dec 5 11:35:14 2025 -0500 add MiniDumpLocalVarLookup test (dotnet#5579) * Adds SOS test for minidump local var lookup commit 848b19c Author: Max Charlamb <[email protected]> Date: Thu Dec 4 22:37:26 2025 -0500 Simplify test `csproj` files (dotnet#5653) * MSBuild property: `$(SrcDir) -> $(RepoRoot)/src/` * MSBuild property: `$(TestDir) -> $(ReporRoot/src/tests/` * Tests `.csproj` files to use these properties to simplify imports. This also allows us to move around the test/src directories without modifying each project individually. * `Microsoft.FileFormats.UnitTests.csproj` and `Microsoft.SymbolStore.UnitTests.csproj` both copy test binary assets from the source tree into the artifacts tree. This was handled individually for each item with some inconsistencies. I simplified this process using globbing. There are a couple more files that are copied now, but I believe this shouldn't have any appreciable impact. commit bc69097 Author: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Date: Thu Dec 4 16:20:38 2025 -0800 [main] Update dependencies from dotnet/dotnet (dotnet#5650) This pull request updates the following dependencies [marker]: <> (Begin:9c7d85bb-274e-4ad6-970a-48ffc448929b) - **Subscription**: [9c7d85bb-274e-4ad6-970a-48ffc448929b](https://maestro.dot.net/subscriptions?search=9c7d85bb-274e-4ad6-970a-48ffc448929b) - **Build**: [20251203.3](https://dev.azure.com/dnceng/internal/_build/results?buildId=2852778) ([293194](https://maestro.dot.net/channel/5173/github:dotnet:dotnet/build/293194)) - **Date Produced**: December 3, 2025 7:24:47 PM UTC - **Commit**: [5ddd0ddc0ebadca21645a05c419ed5a034454605](dotnet/dotnet@5ddd0dd) - **Branch**: [release/10.0.1xx](https://github.com/dotnet/dotnet/tree/release/10.0.1xx) [DependencyUpdate]: <> (Begin) - **Dependency Updates**: - From [10.0.2-servicing.25601.110 to 10.0.2-servicing.25603.103][1] - runtime.linux-arm64.Microsoft.DotNet.Cdac.Transport - runtime.linux-x64.Microsoft.DotNet.Cdac.Transport - runtime.osx-arm64.Microsoft.DotNet.Cdac.Transport - runtime.osx-x64.Microsoft.DotNet.Cdac.Transport - runtime.win-arm64.Microsoft.DotNet.Cdac.Transport - Microsoft.AspNetCore.App.Ref.Internal - Microsoft.NETCore.Platforms - runtime.win-x64.Microsoft.DotNet.Cdac.Transport - From [10.0.2 to 10.0.2][1] - Microsoft.AspNetCore.App.Ref - Microsoft.NETCore.App.Ref - From [5.0.0-2.25601.110 to 5.0.0-2.25603.103][1] - Microsoft.CodeAnalysis - Microsoft.CodeAnalysis.Analyzers - Microsoft.CodeAnalysis.CSharp - From [10.0.102 to 10.0.102][1] - Microsoft.CodeAnalysis.NetAnalyzers - From [10.0.0-beta.25601.110 to 10.0.0-beta.25603.103][1] - Microsoft.DotNet.Arcade.Sdk - Microsoft.DotNet.CodeAnalysis - From [10.0.102-servicing.25601.110 to 10.0.102-servicing.25603.103][1] - Microsoft.NET.Sdk [1]: dotnet/dotnet@7dedd35...5ddd0dd [DependencyUpdate]: <> (End) [marker]: <> (End:9c7d85bb-274e-4ad6-970a-48ffc448929b) Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> commit 4ba3f42 Author: Max Charlamb <[email protected]> Date: Thu Dec 4 18:53:44 2025 -0500 Remove references to $(RepoRoot) in testing scripts (dotnet#5614) This brings us closer to sending the tests to helix commit a81816d Author: Max Charlamb <[email protected]> Date: Thu Dec 4 13:29:25 2025 -0500 move lldbplugin.tests to test folder (dotnet#5651) commit 94caedf Author: Max Charlamb <[email protected]> Date: Thu Dec 4 13:07:42 2025 -0500 Remove `FreeLibrary` invocations in managed SOS host (dotnet#5586) * Fixes intermittent failures when using the cDAC * Prepares SOS to consume NativeAOT packages
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
dotnet/runtime#117785, the
runtime-diagnosticspipeline has had intermittent failures due to SOS freeing the DAC/cDAC library and causing a double free race condition.NativeAOT binaries do not support being unloaded, therefore this will be required for future cDAC use.
This change doesn't impact the native host, but in most scenarios the managed host is favored.