Refactor System.Net.* AppContextSwitch usage to LocalAppContextSwitches#125385
Refactor System.Net.* AppContextSwitch usage to LocalAppContextSwitches#125385rzikm wants to merge 12 commits intodotnet:mainfrom
Conversation
Replace inline AppContext.TryGetSwitch + environment variable patterns with AppContextSwitchHelper.GetBooleanConfig() calls in: - SocketProtocolSupportPal (System.Net.Sockets, Ping, NameResolution) - SslStream.Protocol (DisableTlsResume, EnableServerAiaDownloads) - AuthenticationHelper.NtAuth (UsePortInSpn) - SslAuthenticationOptions (OCSP stapling switch) - HttpListener.Windows (EnableKernelResponseBuffering) - SslKeyLogger (EnableSslKeyLogging) - NegotiateAuthenticationPal.Unix (UseManagedNtlm) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extend LocalAppContextSwitches.Common.cs with a GetCachedSwitchValue overload that supports environment variable fallback, and add GetBooleanEnvironmentVariable helper method. Create per-library LocalAppContextSwitches.cs files: - System.Net.Security: DisableTlsResume, EnableServerAiaDownloads, EnableOcspStapling - System.Net.Http: UsePortInSpn - System.Net.HttpListener: EnableKernelResponseBuffering This replaces the ad-hoc NullableBool caching pattern with the standard LocalAppContextSwitches pattern used across the codebase (System.Data.Common, System.Diagnostics.DiagnosticSource, etc.). NullableBool.cs is no longer needed in Http and Security. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move DisableIPv6 from SocketProtocolSupportPal.cs to a new shared Common/src/System/Net/LocalAppContextSwitches.Net.cs file, using the GetSwitchValue + GetBooleanEnvironmentVariable pattern for single-evaluation switches with env var fallback. Move UseManagedNtlm from NegotiateAuthenticationPal.Unix.cs to the System.Net.Security LocalAppContextSwitches.cs, using the same GetSwitchValue pattern with a computed default value for platforms that default to managed NTLM. Replace AppContextSwitchHelper.cs with LocalAppContextSwitches in Sockets, Ping, and NameResolution csprojs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the same GetCachedSwitchValue pattern with an int backing field as all other networking LocalAppContextSwitches properties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add GetCachedSwitchValue overload with a defaultValue parameter to LocalAppContextSwitches.Common.cs for switches that have a non-false default. Switch UseManagedNtlm to use the same cached int field pattern as all other networking switches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tches Move UseNetworkFramework and EnableSslKeyLogging switches to LocalAppContextSwitches in System.Net.Security. Move DisableConfigurationCache and AppLocalMsQuic switches to a new LocalAppContextSwitches.cs in System.Net.Quic. Remove AppContextSwitchHelper.cs from Security, Quic, and Security unit test csprojs — all networking AppContext switches now use the LocalAppContextSwitches pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delete the file and remove all remaining csproj references (WinHttpHandler src and tests had unused includes). System.Text.Json has its own local copy which is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move default values to call sites using the defaultValue parameter. Merge the two no-envvar GetCachedSwitchValue overloads into one with 'bool defaultValue = false'. Remove GetSwitchDefaultValue and one GetCachedSwitchValueInternal overload. Affected callers updated to pass defaultValue: true: - SerializationGuard (System.Private.CoreLib) - BinaryFormatterEnabled (System.Runtime.Serialization.Formatters) - IsNetworkingEnabledByDefault (System.Private.Xml) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use a single GetCachedSwitchValueInternal with a nullable envVariable parameter instead of two separate overloads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR refactors AppContext switch handling across System.Net.* assemblies by consolidating the various patterns (manual AppContext.TryGetSwitch, NullableBool state machines, and AppContextSwitchHelper) into a unified LocalAppContextSwitches partial class pattern. The common base (LocalAppContextSwitches.Common.cs) is extended with environment variable fallback support, and each assembly defines its specific switches in a local partial class file.
Changes:
- Replaced
AppContextSwitchHelperand manualAppContext.TryGetSwitch/NullableBoolpatterns withLocalAppContextSwitchespartial class pattern across System.Net.Security, System.Net.Quic, System.Net.Http, System.Net.HttpListener, and shared common code. - Extended
LocalAppContextSwitches.Common.cswith environment variable fallback support and moved default value handling from a centralizedGetSwitchDefaultValuemethod to inlinedefaultValueparameters at each call site. - Updated all
.csprojfiles to reference the new common files and removed references to the now-deletedAppContextSwitchHelper.csand unnecessaryNullableBool.cs.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
Common/src/System/LocalAppContextSwitches.Common.cs |
Extended with env variable fallback, new overloads, and TryGetBooleanEnvironmentVariable helper; removed GetSwitchDefaultValue |
Common/src/System/Net/LocalAppContextSwitches.Net.cs |
New file: shared net-level switches (DisableIPv6, EnableSslKeyLogging) |
Common/src/System/AppContextSwitchHelper.cs |
Deleted: replaced by LocalAppContextSwitches pattern |
Common/src/System/Net/Security/SslKeyLogger.cs |
Replaced #if DEBUG / AppContext.TryGetSwitch with LocalAppContextSwitches.EnableSslKeyLogging |
Common/src/System/Net/SocketProtocolSupportPal.cs |
Replaced manual IPv6 disabled check with LocalAppContextSwitches.DisableIPv6 |
Common/src/Interop/Unix/.../Interop.OpenSsl.cs |
Updated DisableTlsResume reference to LocalAppContextSwitches |
System.Net.Security/src/.../LocalAppContextSwitches.cs |
New file: Security-specific switches (DisableTlsResume, EnableServerAiaDownloads, EnableOcspStapling, UseManagedNtlm, UseNetworkFramework) |
System.Net.Security/src/.../SslStream.Protocol.cs |
Removed inline switch implementations; updated references |
System.Net.Security/src/.../SslStreamPal.Windows.cs |
Updated DisableTlsResume references |
System.Net.Security/src/.../SslAuthenticationOptions.cs |
Updated OCSP stapling switch usage |
System.Net.Security/src/.../SafeDeleteNwContext.cs |
Updated UseNetworkFramework switch |
System.Net.Security/src/.../NegotiateAuthenticationPal.Unix.cs |
Removed local UseManagedNtlm; uses LocalAppContextSwitches |
System.Net.Security/src/System.Net.Security.csproj |
Updated project references |
System.Net.Security/tests/.../System.Net.Security.Unit.Tests.csproj |
Updated test project references |
System.Net.Quic/src/.../LocalAppContextSwitches.cs |
New file: Quic-specific switches (DisableConfigurationCache, AppLocalMsQuic) |
System.Net.Quic/src/.../MsQuicConfiguration.Cache.cs |
Updated ConfigurationCacheEnabled |
System.Net.Quic/src/.../MsQuicApi.cs |
Updated ShouldUseAppLocalMsQuic |
System.Net.Quic/src/System.Net.Quic.csproj |
Updated project references |
System.Net.Http/src/.../LocalAppContextSwitches.cs |
New file: Http-specific switch (UsePortInSpn) |
System.Net.Http/src/.../AuthenticationHelper.NtAuth.cs |
Removed inline UsePortInSpn switch; uses LocalAppContextSwitches |
System.Net.Http/src/System.Net.Http.csproj |
Updated project references |
System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj |
Removed unused AppContextSwitchHelper reference |
System.Net.Http.WinHttpHandler/tests/.../Unit.Tests.csproj |
Removed unused AppContextSwitchHelper reference |
System.Net.HttpListener/src/.../LocalAppContextSwitches.cs |
New file: HttpListener-specific switch (EnableKernelResponseBuffering) |
System.Net.HttpListener/src/.../HttpListener.Windows.cs |
Updated reference |
System.Net.HttpListener/src/System.Net.HttpListener.csproj |
Updated project references |
System.Net.Sockets/src/System.Net.Sockets.csproj |
Added new common file references |
System.Net.Ping/src/System.Net.Ping.csproj |
Added new common file references |
System.Net.NameResolution/src/System.Net.NameResolution.csproj |
Added new common file references |
System.Net.NameResolution/tests/.../Pal.Tests.csproj |
Added new common file references |
System.Private.CoreLib/src/.../LocalAppContextSwitches.cs |
Updated SerializationGuard to use defaultValue parameter |
System.Runtime.Serialization.Formatters/src/.../LocalAppContextSwitches.cs |
Updated BinaryFormatterEnabled to use defaultValue parameter |
System.Private.Xml/src/.../LocalAppContextSwitches.cs |
Updated IsNetworkingEnabledByDefault to use defaultValue parameter |
Comments suppressed due to low confidence (1)
src/libraries/Common/src/System/LocalAppContextSwitches.Common.cs:63
- The
outparametervalueis not assigned on thefalsereturn path (line 63). C# requires alloutparameters to be definitely assigned before the method returns. This will cause a compilation error.
Add value = false; before return false;.
return false;
You can also share your feedback on Copilot code review. Take the survey.
| private static int s_disableIPv6; | ||
| internal static bool DisableIPv6 | ||
| { | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| get => GetCachedSwitchValue("System.Net.DisableIPv6", "DOTNET_SYSTEM_NET_DISABLEIPV6", ref s_disableIPv6); | ||
| } |
There was a problem hiding this comment.
While we're changing things, can we use a pattern more like this?
public static bool DisableIPv6 { get; } = GetSwitchValue("System.Net.DisableIPv6", "DOTNET_SYSTEM_NET_DISABLEIPV6");This way the JIT can see that the value can't change, and completely remove unreachable branches when tiering up.
There was a problem hiding this comment.
This pattern defeats the purpose of TestSwitch.LocalAppContext.DisableCaching that allows caching to be disabled and turn the appcontext switch on/off without restarting the process.
Also, this pattern is not friendly for startup and trimming: It will read all switches eagerly, even if they are not used by the app. This startup performance hit may be acceptable for System.Net.*, but we would not want to spread it everywhere (CoreLib in particular).
Add missing return statement and fast-path cache check in the GetCachedSwitchValue overload without envVariable parameter. Also fix missing out parameter assignment in TryGetBooleanEnvironmentVariable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace instances of AppContextSwitchHelper with LocalAppContextSwitches across System.Net.* assemblies. This change standardizes the handling of configuration switches and improves code maintainability by adopting a consistent pattern for switch management.
This PR does not touch appcontext switches in HTTP code, as it also uses numeric values and has separate implementation in RuntimeSettingsParser helper.