InternalLogger - Auto enable LogLevel.Info when activated#6070
InternalLogger - Auto enable LogLevel.Info when activated#6070
Conversation
WalkthroughInternalLogger now uses a nullable backing for its log level and lazily initializes it (to Info) when writers or console/file flags are enabled; LogWriter gained a backing field. LoggingConfigurationParser no longer auto-disables InternalLogger. Several tests were updated to call InternalLogger.Reset() and minor test cleanups performed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-12T01:22:52.437ZApplied to files:
🧬 Code graph analysis (1)src/NLog/Common/InternalLogger.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
snakefoot
force-pushed
the
internallogger_info
branch
9 times, most recently
from
e044f4d to
5c3796f
Compare
snakefoot
force-pushed
the
internallogger_info
branch
2 times, most recently
from
24b9da3 to
391ad42
Compare
snakefoot
changed the title
snakefoot
changed the title
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NLog/Common/InternalLogger.cs (1)
85-102: Potential race condition in auto-enable logic.The check-and-set pattern at lines 96-97 is not atomic. If multiple threads concurrently enable console logging during initialization, there's a race on
_logLevel. However, since the InternalLogger is typically configured once at startup and the race would only result in redundant assignments toInfo(not corruption), this may be acceptable.Consider whether explicit locking or
Interlocked.CompareExchangeis needed based on your expected usage patterns.
♻️ Duplicate comments (2)
src/NLog/Common/InternalLogger.cs (2)
109-126: Same race condition applies here.Lines 120-121 have the same non-atomic check-and-set pattern as
LogToConsole. The same analysis applies: likely acceptable given typical usage, but worth considering for consistency.
133-161: Same race condition applies here.Lines 148-149 have the same non-atomic check-and-set pattern. Consistent with the other output setters.
🧹 Nitpick comments (1)
src/NLog/Common/InternalLogger.cs (1)
428-431: Consider more idiomatic null check.The logic at line 430 is correct, but
!ReferenceEquals(_logLevel, null)is less idiomatic than_logLevel is not nullor_logLevel != null. WhileReferenceEqualsis functionally equivalent (and avoids operator overloading), modern C# favors pattern matching for readability.🔎 Suggested refactor
-return !ReferenceEquals(_logLevel, null) && _logLevel.CompareTo(logLevel) <= 0; +return _logLevel is not null && _logLevel.CompareTo(logLevel) <= 0;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog/Common/InternalLogger.cssrc/NLog/Config/LoggingConfigurationParser.cstests/NLog.UnitTests/Config/XmlConfigTests.cs
💤 Files with no reviewable changes (1)
- src/NLog/Config/LoggingConfigurationParser.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Common/InternalLogger.cs (1)
src/NLog/LogLevel.cs (5)
LogLevel(136-140)LogLevel(253-275)LogLevel(282-337)CompareTo(389-392)CompareTo(404-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (39)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (5)
src/NLog/Common/InternalLogger.cs (3)
58-68: LGTM: Reset correctly initializes to unconfigured state.The change to set
_logLevel = null(instead ofLogLevel.Off) correctly implements the "unconfigured" semantics, allowing the auto-enable behavior to work when outputs are later configured.
74-79: LGTM: Nullable backing field enables lazy initialization.The nullable
_logLevelbacking field elegantly distinguishes between "unconfigured" (null → defaults to Off) and "explicitly disabled" (set to Off), enabling the auto-enable behavior when outputs are configured.
315-323: LGTM: Efficient early return.The check at line 320 correctly short-circuits when there are no output destinations, avoiding unnecessary string formatting. Using the
_logWriterbacking field is appropriate here.tests/NLog.UnitTests/Config/XmlConfigTests.cs (2)
47-64: LGTM: Test correctly validates unconfigured state.The
InternalLogger.Reset()call at line 51 ensures test isolation. The assertion at line 62 correctly verifies that when no internal logging outputs are configured, the log level remainsOff(i.e.,_logLevelis null and the property defaults toOff). The comment clarifies the expected behavior.
67-87: LGTM: Test correctly validates auto-enable behavior.The
InternalLogger.Reset()call at line 71 ensures clean state. The assertion at line 84 correctly verifies that when console outputs are enabled (line 75), the internal log level is auto-enabled toInfo. This validates the core feature of this PR. The comment clarifies the expected behavior.
| public static TextWriter? LogWriter | ||
| { | ||
| get => _logWriter; | ||
| set | ||
| { | ||
| if (value != null) | ||
| { | ||
| if (_logLevel is null) | ||
| _logLevel = LogLevel.Info; | ||
| } | ||
| _logWriter = value; | ||
| } | ||
| } | ||
| private static TextWriter? _logWriter; |
There was a problem hiding this comment.
Backing field approach is sound; same race condition applies.
Using _logWriter as a backing field is correct and avoids re-triggering setter logic in WriteToLog. The auto-enable pattern at lines 174-175 has the same race condition as the other output setters.
🤖 Prompt for AI Agents
In src/NLog/Common/InternalLogger.cs around lines 167 to 180, the LogWriter
setter uses the backing field correctly but the auto-enable pattern (if
(_logLevel is null) _logLevel = LogLevel.Info;) is vulnerable to the same race
condition as other output setters; change this to perform the enable under the
same synchronization used elsewhere (either wrap the null-check-and-set in the
existing lock/sync object used by other setters or perform an atomic
compare-and-set on the underlying storage) so that only one thread can observe
and set _logLevel to Info; keep using the backing field _logWriter, but ensure
the _logLevel assignment is done atomically/under lock to avoid races.
391ad42 to
71b385a
Compare
snakefoot
force-pushed
the
internallogger_info
branch
from
71b385a to
eaf866e
Compare
|
1 participant
Partially reverts #4547 and #2935 - Auto-enable LogLevel.Info unless explicitly configured to LogLevel.Off.
Has the side-effect that explicit configuring
internalLogLevel="Off"gives a minor performance hit.Also has the side-effect that if having explicit configured
internalLogToConsole="true"orinternalLogToConsoleError="true"orinternalLogFile="...", then logging is now automatically activated without having assignedinternalLogLevel(Similar behavior as NLog v4.7.15 and older)