Fix race in Hierarchy.TryCreateLogger#294
Open
gdziadkiewicz wants to merge 7 commits intoapache:masterfrom
Open
Fix race in Hierarchy.TryCreateLogger#294gdziadkiewicz wants to merge 7 commits intoapache:masterfrom
gdziadkiewicz wants to merge 7 commits intoapache:masterfrom
Conversation
Co-authored-by: Copilot <copilot@github.com>
Contributor
Author
|
Two of the new tests are on the slow list :
|
There was a problem hiding this comment.
Pull request overview
This pull request addresses a concurrency regression in Hierarchy.GetLogger(...) where concurrent logger creation could return an unregistered Logger, causing events to be dropped. It restores synchronization around logger creation/registration and adds regression tests based on the reproduction in #292.
Changes:
- Reintroduces a lock in
Hierarchy.TryCreateLoggerto serialize logger creation + registration, avoiding returning partially registered loggers. - Introduces an internal
IHierarchyNodemarker so the logger map can store eitherLoggerorProvisionNodein a type-safe way. - Adds concurrency and repository enumeration regression tests, including cases with dotted logger names and failure scenarios in the logger-creation event.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/log4net/Repository/Hierarchy/ProvisionNode.cs |
Adds IHierarchyNode and implements it for ProvisionNode to support typed storage in the hierarchy map. |
src/log4net/Repository/Hierarchy/Logger.cs |
Implements IHierarchyNode on Logger to unify dictionary value types. |
src/log4net/Repository/Hierarchy/Hierarchy.cs |
Fixes the race by locking around logger creation/registration and updates hierarchy node handling to use IHierarchyNode. |
src/log4net.Tests/Hierarchy/LoggingConcurrencyTest.cs |
Adds regression tests to validate no message loss and correct blocking behavior during concurrent logger creation/registration. |
src/log4net.Tests/Hierarchy/HierarchyTest.cs |
Adds tests ensuring GetCurrentLoggers() behavior (empty, includes created, excludes root, excludes provision nodes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return existingLogger; | ||
| // Need to create a new logger and register it, but there is already a provision node for it | ||
| case ProvisionNode provisionNode: | ||
| // Locking to stop the changes while we try to replace the provisions node with a logger |
Comment on lines
+185
to
+193
| Task childLogTask = Task.Run(() => | ||
| { | ||
| Logger childLogger = (Logger)hierarchy.GetLogger(secondChildName); | ||
| childLogger.Log(Level.Info, "Message from child while parent registers", null); | ||
| }); | ||
|
|
||
| Assert.That(childLogTask.Wait(TimeSpan.FromMilliseconds(100)), Is.False, | ||
| "The child logger was returned before its parent logger was ready."); | ||
|
|
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.
This PR adds the regression (compared to log4net version 2) test case from #292 and fix in form of bringing back lock in Hierarchy.TryCreateLogger that is used to synchronize creation and registering of new Loggers. Prior to the change it was possible to get an unregistered logger that will result in dropped messages.
Resolves #292