Copilot commented on code in PR #306: URL: https://github.com/apache/logging-log4net/pull/306#discussion_r3764546519
########## src/log4net/Diagnostics/CodeAnalysis/DynamicallyAccessedMemberTypes.cs: ########## @@ -0,0 +1,53 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +// inspired by https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/DynamicallyAccessedMemberTypes.cs + +#if !NET6_0_OR_GREATER +namespace System.Diagnostics.CodeAnalysis; + +/// <summary> +/// Specifies the types of members that are dynamically accessed. +/// </summary> +/// <remarks> +/// <para> +/// The trimmer matches this type by its full name rather than by identity, so the values have to +/// keep the numbering the framework uses. Only the members log4net annotates with are declared; +/// add further ones from the runtime source above as they are needed. +/// </para> +/// </remarks> +[Flags] +internal enum DynamicallyAccessedMemberTypes +{ + /// <summary> + /// Specifies no members. + /// </summary> + None = 0, + + /// <summary> + /// Specifies the default, parameterless public constructor. + /// </summary> + PublicParameterlessConstructor = 0x0001, + + /// <summary> + /// Specifies all public constructors. + /// </summary> + PublicConstructors = 0x0002 | PublicParameterlessConstructor, Review Comment: `DynamicallyAccessedMemberTypes` is documented here as needing to match the framework's numeric values, but `PublicConstructors` is defined as `0x0002 | PublicParameterlessConstructor` (i.e., `0x0003`). If this enum is ever used, that mismatch can lead to incorrect trimming behavior or confusion when comparing against framework values. Consider matching the framework definition exactly (i.e., keep `PublicConstructors` at its framework bit value, and let callers OR flags explicitly if they want combinations). ########## src/log4net/Util/SystemInfo.cs: ########## @@ -680,20 +683,90 @@ public static bool TryParse(string s, out short val) /// <returns>the value for the key, or <see langword="null"/></returns> public static string? GetAppSetting(string key) { - if (IsAndroid) - return Environment.GetEnvironmentVariable(key); // Android does not support config files + // Android does not support config files, and neither does a runtime that has trimmed the + // configuration system away. + if (IsAndroid || _configurationSystemUnavailable) + return Environment.GetEnvironmentVariable(key); try { - return ConfigurationManager.AppSettings[key]; + return ReadAppSetting(key); } catch (Exception e) when (!e.IsFatal()) { - // If an exception is thrown here then it looks like the config file does not parse correctly. + if (IsMissingConfigurationSystem(e)) + { + // There is no configuration system to read - Native AOT trims System.Configuration away. + // That is a property of the runtime rather than a fault, so it is not reported as an + // error, and the environment stands in for the config file as it does on Android. + _configurationSystemUnavailable = true; Review Comment: `_configurationSystemUnavailable` is used as a cross-call/site latch but is a plain static `bool`. In multi-threaded scenarios, this can lead to repeated attempts/logging (lost visibility) or inconsistent behavior across threads. Consider making this field `volatile` and/or using `Interlocked.Exchange/CompareExchange` so the transition to 'unavailable' is safely published and the 'first failure logs once' behavior is reliable. ########## src/log4net/Util/SystemInfo.cs: ########## @@ -680,20 +683,90 @@ public static bool TryParse(string s, out short val) /// <returns>the value for the key, or <see langword="null"/></returns> public static string? GetAppSetting(string key) { - if (IsAndroid) - return Environment.GetEnvironmentVariable(key); // Android does not support config files + // Android does not support config files, and neither does a runtime that has trimmed the + // configuration system away. + if (IsAndroid || _configurationSystemUnavailable) + return Environment.GetEnvironmentVariable(key); try { - return ConfigurationManager.AppSettings[key]; + return ReadAppSetting(key); } catch (Exception e) when (!e.IsFatal()) { - // If an exception is thrown here then it looks like the config file does not parse correctly. + if (IsMissingConfigurationSystem(e)) + { + // There is no configuration system to read - Native AOT trims System.Configuration away. + // That is a property of the runtime rather than a fault, so it is not reported as an + // error, and the environment stands in for the config file as it does on Android. + _configurationSystemUnavailable = true; + LogLog.Debug(_declaringType, + "No configuration system on this runtime. Using environment variables for application settings.", e); + return Environment.GetEnvironmentVariable(key); + } + + // The config file itself does not parse. Report it and treat the setting as absent, without + // falling back to the environment - a broken config file must not silently change where + // settings come from. LogLog.Error(_declaringType, "Exception while reading ConfigurationSettings. Check your .config file is well formed XML.", e); } return null; } + /// <summary> + /// Determines whether <paramref name="exception"/> means that there is no configuration system + /// on this runtime, as opposed to a configuration file that does not parse. + /// </summary> + /// <param name="exception">the exception thrown while reading an application setting</param> + /// <returns><see langword="true"/> if the configuration system itself is unavailable</returns> + /// <remarks> + /// <para> + /// The inner exceptions have to be walked, because Native AOT surfaces this as a + /// <see cref="ConfigurationErrorsException"/> - the very type a malformed file produces. What + /// distinguishes it is further down the chain: a <see cref="MissingMethodException"/> for + /// <c>ClientConfigurationHost</c>, whose constructor the trimmer removed. + /// </para> + /// <para> + /// An unrecognized failure is treated as a configuration file problem, which is the safer way + /// round: it is reported rather than silently swallowed. + /// </para> + /// </remarks> + private static bool IsMissingConfigurationSystem(Exception? exception) + { + for (; exception is not null; exception = exception.InnerException) + { + if (exception is MissingMethodException or TypeLoadException or FileNotFoundException + or PlatformNotSupportedException or NotSupportedException) + { + return true; + } + } + return false; + } + + /// <summary> + /// Reads a single application setting. + /// </summary> + /// <param name="key">the application settings key to lookup</param> + /// <returns>the value for the key, or <see langword="null"/></returns> + /// <remarks> + /// <para> + /// Separate from <see cref="GetAppSetting"/>, and never inlined into it, so that the failure to + /// resolve <see cref="ConfigurationManager"/> itself is raised on entry to this method - inside + /// the caller's try block - rather than on entry to <see cref="GetAppSetting"/>, where nothing + /// would catch it and a <see cref="FileNotFoundException"/> would escape the static constructor + /// as a <see cref="TypeInitializationException"/>. + /// </para> + /// <para> + /// The package declares a dependency on System.Configuration.ConfigurationManager, so this only + /// arises where the assembly is deployed by other means than the package - it costs one method + /// to keep those deployments running instead of failing at type initialization. + /// </para> + /// </remarks> + [MethodImpl(MethodImplOptions.NoInlining)] + private static string? ReadAppSetting(string key) => ConfigurationManager.AppSettings[key]; + + private static bool _configurationSystemUnavailable; Review Comment: `_configurationSystemUnavailable` is used as a cross-call/site latch but is a plain static `bool`. In multi-threaded scenarios, this can lead to repeated attempts/logging (lost visibility) or inconsistent behavior across threads. Consider making this field `volatile` and/or using `Interlocked.Exchange/CompareExchange` so the transition to 'unavailable' is safely published and the 'first failure logs once' behavior is reliable. ########## src/log4net/Layout/PatternLayout.cs: ########## @@ -983,14 +978,9 @@ protected virtual PatternParser CreatePatternParser(string pattern) PatternParser patternParser = new(pattern); // Add all the builtin patterns - foreach (KeyValuePair<string, Type> entry in _sGlobalRulesRegistry) + foreach (KeyValuePair<string, ConverterInfo> entry in _sGlobalRulesRegistry) { - ConverterInfo converterInfo = new() - { - Name = entry.Key, - Type = entry.Value - }; - patternParser.PatternConverters[entry.Key] = converterInfo; + patternParser.PatternConverters[entry.Key] = entry.Value; } Review Comment: This now shares the same mutable `ConverterInfo` instances (with settable properties) across all `PatternParser` instances. Previously, each parser received a fresh `ConverterInfo`, which avoided any accidental cross-layout side effects if a `ConverterInfo` is ever mutated after registration. To preserve the previous isolation while keeping trimming annotations, consider cloning `ConverterInfo` per parser (copy `Name`/`Type`) rather than reusing the global instances (and apply the same approach to `Util/PatternString.cs`, which uses the same pattern). ########## src/log4net.Tests/log4net.Tests.csproj: ########## @@ -19,6 +19,7 @@ <VSTestLogger>quackers</VSTestLogger> </PropertyGroup> <ItemGroup> + <Compile Include="..\log4net\Util\CallerAssembly.cs" Link="Util\CallerAssembly.cs" /> Review Comment: Linking `CallerAssembly.cs` into the test assembly means tests exercise a separately-compiled copy of `log4net.Util.CallerAssembly` rather than the production type in the log4net assembly (different assembly identity can affect `Fallback`, and conditional compilation symbols can diverge). If the goal is to test the real internal implementation, consider using `InternalsVisibleTo` so tests can reference the production `CallerAssembly` directly, avoiding duplication and assembly-identity differences. ########## src/log4net/Util/SystemInfo.cs: ########## @@ -680,20 +683,90 @@ public static bool TryParse(string s, out short val) /// <returns>the value for the key, or <see langword="null"/></returns> public static string? GetAppSetting(string key) { - if (IsAndroid) - return Environment.GetEnvironmentVariable(key); // Android does not support config files + // Android does not support config files, and neither does a runtime that has trimmed the + // configuration system away. + if (IsAndroid || _configurationSystemUnavailable) + return Environment.GetEnvironmentVariable(key); try { - return ConfigurationManager.AppSettings[key]; + return ReadAppSetting(key); } catch (Exception e) when (!e.IsFatal()) { - // If an exception is thrown here then it looks like the config file does not parse correctly. + if (IsMissingConfigurationSystem(e)) + { + // There is no configuration system to read - Native AOT trims System.Configuration away. + // That is a property of the runtime rather than a fault, so it is not reported as an + // error, and the environment stands in for the config file as it does on Android. + _configurationSystemUnavailable = true; + LogLog.Debug(_declaringType, + "No configuration system on this runtime. Using environment variables for application settings.", e); + return Environment.GetEnvironmentVariable(key); + } + + // The config file itself does not parse. Report it and treat the setting as absent, without + // falling back to the environment - a broken config file must not silently change where + // settings come from. LogLog.Error(_declaringType, "Exception while reading ConfigurationSettings. Check your .config file is well formed XML.", e); } return null; } + /// <summary> + /// Determines whether <paramref name="exception"/> means that there is no configuration system + /// on this runtime, as opposed to a configuration file that does not parse. + /// </summary> + /// <param name="exception">the exception thrown while reading an application setting</param> + /// <returns><see langword="true"/> if the configuration system itself is unavailable</returns> + /// <remarks> + /// <para> + /// The inner exceptions have to be walked, because Native AOT surfaces this as a + /// <see cref="ConfigurationErrorsException"/> - the very type a malformed file produces. What + /// distinguishes it is further down the chain: a <see cref="MissingMethodException"/> for + /// <c>ClientConfigurationHost</c>, whose constructor the trimmer removed. + /// </para> + /// <para> + /// An unrecognized failure is treated as a configuration file problem, which is the safer way + /// round: it is reported rather than silently swallowed. + /// </para> + /// </remarks> + private static bool IsMissingConfigurationSystem(Exception? exception) + { + for (; exception is not null; exception = exception.InnerException) + { + if (exception is MissingMethodException or TypeLoadException or FileNotFoundException + or PlatformNotSupportedException or NotSupportedException) + { + return true; + } + } + return false; + } Review Comment: `IsMissingConfigurationSystem` is currently broad enough that *any* `FileNotFoundException` / `TypeLoadException` anywhere in the exception chain will cause log4net to treat the configuration system as missing and silently switch to environment variables (and only log at Debug). This can mask real configuration problems (e.g., a malformed config or a config referencing an optional/absent assembly/handler) and change behavior from 'error + null' to 'debug + env fallback'. Recommend narrowing detection to known 'configuration system missing/trimmed' signatures (e.g., missing `System.Configuration.ConfigurationManager` assembly, or the specific trimmed type/ctor like `System.Configuration.ClientConfigurationHost`), and otherwise keep reporting as a config-file error. ########## src/log4net/Util/SystemInfo.cs: ########## @@ -680,20 +683,90 @@ public static bool TryParse(string s, out short val) /// <returns>the value for the key, or <see langword="null"/></returns> public static string? GetAppSetting(string key) { - if (IsAndroid) - return Environment.GetEnvironmentVariable(key); // Android does not support config files + // Android does not support config files, and neither does a runtime that has trimmed the + // configuration system away. + if (IsAndroid || _configurationSystemUnavailable) + return Environment.GetEnvironmentVariable(key); Review Comment: `_configurationSystemUnavailable` is used as a cross-call/site latch but is a plain static `bool`. In multi-threaded scenarios, this can lead to repeated attempts/logging (lost visibility) or inconsistent behavior across threads. Consider making this field `volatile` and/or using `Interlocked.Exchange/CompareExchange` so the transition to 'unavailable' is safely published and the 'first failure logs once' behavior is reliable. ########## src/log4net.Tests/Util/SystemInfoTest.cs: ########## @@ -171,4 +173,102 @@ public void EqualsIgnoringCase_DifferentStrings_false() [Platform(Include = "Win,Linux,MacOsX")] public void IsAndoid() => Assert.That(typeof(SystemInfo).GetProperty("IsAndroid", BindingFlags.Static | BindingFlags.NonPublic)?.GetValue(null), Is.False); + + /// <summary> + /// <see cref="SystemInfo.GetAppSetting"/> falls back to environment variables once the + /// configuration system has failed - which is what happens under Native AOT, where + /// System.Configuration is trimmed away. + /// </summary> + /// <remarks> + /// <para> + /// That failure cannot be provoked on a JIT runtime, so the latch that records it is flipped + /// directly, the same way <see cref="IsAndoid"/> reaches a non-public member. The environment + /// must stay untouched while the configuration system still works, otherwise a malformed Review Comment: The documentation references `IsAndoid` (typo). Renaming the test to `IsAndroid` (and updating this `<see cref=.../>`) would improve clarity and avoid propagating the typo in new comments. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
