Expose logger properties on Logger#3424
Expose logger properties on Logger#3424304NotModified wants to merge 3 commits intorelease/4.6.4from
Conversation
Codecov Report
@@ Coverage Diff @@
## release/4.6.4 #3424 +/- ##
==============================================
- Coverage 80% 80% -<1%
==============================================
Files 358 358
Lines 28373 28517 +144
Branches 3785 3814 +29
==============================================
+ Hits 22750 22838 +88
- Misses 4537 4583 +46
- Partials 1086 1096 +10 |
| /// <summary> | ||
| /// Properties added with <see cref="WithProperty"/> or <see cref="SetProperty"/> | ||
| /// </summary> | ||
| public IDictionary<string, object> Properties => _contextProperties ?? new Dictionary<string, object>(); |
There was a problem hiding this comment.
You are opening a door to race-condition-hell by returning an unprotected dictionary.
I recommend that you return IReadOnlyDictionary that only works on the platforms where it is known.
There was a problem hiding this comment.
And that race condition isn't there with WithProperty/SetProperty
There was a problem hiding this comment.
I know WithProperty/SetProperty are nice and sweet (because I made them so). But the evil user that starts calling the IDictionary-inteface to insert/remove elements will break everything.
There was a problem hiding this comment.
Sorry, I don't see what it should break. Could you please give an example ?
There was a problem hiding this comment.
Very confused that you don't know what happens if one enumerates a collection while another modifies the collection. Made some pseudo-code in new comment.
var logger = LogManager.GetLogger("Hello").WithProperty("Test", 0);
var thread = new Thread(() => {
for (int i = 1; i < 1000000; ++i)
{
logger.Properties.Remove("Test");
logger.Properties.Add("Test", i);
}
});
thread.Start();
for (int i = 0; i < 1000000; ++i)
logger.Info("Hello World"); // Collection was modified |
|
We could change it to a concurrent dictionary? (Could, not saying we should) |
That would be a bad trade, regarding logging-performance. Since the current reason for adding Properties-properties is to make it easier to see the current values. And the logger will be punished on every logevent with concurrentdictionary. Ofcourse if the IDictionary-logic just created for GDC was extracted then we would have a thread-safe idictionary that could be used here. |
|
|
||
| _contextProperties = CopyOnWrite(propertyKey, propertyValue); | ||
| _contextProperties = CreateContextPropertiesDictionary(_contextProperties); | ||
| _contextProperties[propertyKey] = propertyValue; |
There was a problem hiding this comment.
This is not good. As you are modifying the active dictionary.
There was a problem hiding this comment.
not really, CreateContextPropertiesDictionary made a new reference. But anyway, I see this PR is already superseded 👼
|
superseded by #3430 |
fixes #3422 + refactor