omtslug commented on issue #292:
URL:
https://github.com/apache/logging-log4net/issues/292#issuecomment-4325632050
Many thanks for looking into this issue.
Some thoughts/questions I have
( you could very well ignore my 'a' rant, but i had to add it.. )
a, How come classic locks are on the not-use list and should be avoided
(this is not only a log4net trend).
Lock free solutions are great and cool but very hard to reason about, see
e.g. this bug.
Again, never mind about this, it's a subject on its own which we shouldnt
pursue here.
b, In this specific bug we have a race issue when doing concurrent creation
of a logger.
It must be very seldom we actually instantiate a new logger, the
ConcurrentDictionary _loggers.TryGetValue is lock free and
will in all cases except the first return the available logger without
locking.
What I'm proposing is something like this (simplified/naive):
```
if (!_loggers.TryGetValue(key, out object? node))
{
lock (_loggers)
{
if (!_loggers.TryGetValue(key, out node))
{
Logger newLogger = CreateLogger(key.Name);
RegisterLogger(newLogger);
node = _loggers.GetOrAdd(key, newLogger);
}
}
}
```
The exact implementation is not that important, what I want to emphasize
is that if we handle the fast path lock free, ConcurrentDictionary, it
shouldn't be an issue that we take a lock when we need to create/adjust
loggers?
(if needed we could create a logger name specific lock but I'm uncertain
how 'slim/narrow' we can be e.g. in RegisterLogger, UpdateParents etc.)
I see in the v2 code that we take a lock in the GetLogger call and that
will happen both when returning an existing and when needing to create a new
logger.
With the change to ConcurrentDictionary the improvement is good and real,
(but keep a lock for the creation).
c, The background for all this is related to the vulnerability raised some
weeks ago for all log4net versions before 3.3.
We were quite happy with the v2 version.
Is it possible to do a release on v2 major with the vulnerability fixed?
(drop the feature is my suggestion, do what's easiest)
(maybe not realistic, related to when a fix for this can be available and
if there could be other similar issues in v3)
All good and thanks for a great component
/Stefan
--
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]
