I think mixing smart pointers and static class allocation would just mix
things up. We should instead choose when to use one over the other.
We are currently returning new, dynamically allocated level objects for
each call to getInfo() etc. This memory must be freed. When logging from
a logger, a call to getInfo() will allocate a new level object in a
smart pointer, which is returned, used while logging and then when the
smart pointer is destructed the dynamically allocated object is deleted.
We used to have static, dynamically created level objects, that had to
be stored in smart pointers since they were dynamically allocated. Since
they were created at runtime, it wouldn't be thread safe when two calls
would happen simultaneously to the same function.
My suggestion is to have getInfo return a const Level& instead of a
LevelPtr. This would in turn be a reference to a static class member
(allocated without new) meaning its created at compile time. Since it
isn't dynamically created, no smart pointer is needed, and it doesnt
have to be deallocated. This would be more like the initial
pre-LOG4CXX-394 implementation in that the same object is used for each
level, but we would just not have a smart pointer. Since the object is
created at compile time and never modified it would be thread safe, and
would solve LOG4CXX-322 for levels. Loggers cannot use this pattern,
since they are not known at compile time.
/Peter
On 10/27/2016 10:45 AM, Thorsten Schöning wrote:
Guten Tag Peter Westman,
am Donnerstag, 27. Oktober 2016 um 09:05 schrieben Sie:
For levels though, I dont really see why we even have smart pointers? We
could just as easily create the variables statically in the class,
That should be thread-safe regarding LOGCXX-322 as well, shouldn't it?
https://issues.apache.org/jira/browse/LOGCXX-322
and
use them as const references to the original levels.
Do you suggest replacing LevelPtr as the return type of Level::get*?
Because else we seem to need some LevelPtr as static base, because
ObjectPtr-CTORs seem to only be capable of using raw pointers or
ObjectPtrs. But if ObjectPtr leaks now, shouldn't it as well if used
statically in the class? So one would need to find the reason for the
leak anyway?
Maybe we should enhance ObjectPtr to a mode for back compatibility
reasons where a managed pointer is not deleted at all? This way the
interface could stay the same and static levels instead of LevelPtrs
could be used in the class. A new LevelPtr instance would be created
on each invocation of Level::get*, though.
Right now the
library is allocating new dynamic memory on a call basis, which seems
pretty wasteful to me. If we make the storage static (which it was prior
to LOG4CXX-394/r1566655 anyway). In some cases like the Logger class
that stores a changeable level it would have to be a plain pointer, but
I personally dont see a problem with that.
Mit freundlichen Grüßen,
Thorsten Schöning
