Log::Log4perl::Logger->cleanup should not call DESTROY... or exist? #7

Closed
schwern opened this Issue May 9, 2011 · 1 comment

Projects

None yet

2 participants

@schwern
schwern commented May 9, 2011

I was poking around inside the guts of Log::Log4perl and noticed this:

sub cleanup {
    # Delete all loggers
    foreach my $loggername (keys %$LOGGERS_BY_NAME) {
        $LOGGERS_BY_NAME->{$loggername}->DESTROY();
        delete $LOGGERS_BY_NAME->{$loggername};
    }

The explicit call to DESTROY strikes me as an overly aggressive cleanup. If the logger is no longer in use, no longer referenced elsewhere, DESTROY will be called automatically when it's deleted from %LOGGERS_BY_NAME. If the logger IS still referenced, then some code somewhere is left with a busted logger object.

This makes cleanup unusable for long running processes to clean out the cache of unused loggers. See http://stackoverflow.com/questions/5914088/disposing-of-log4perl-logger-when-i-no-longer-need-it

Is there another reason for this? Perhaps to break a circular reference? It was added in 079329e but no rationale is given.

Since Log::Log4perl::Logger->cleanup is undocumented, and its only use is in an END block, and all it does is disassemble some hashes and call DESTROY on some objects that are about to be destroyed anyway... why does it exist at all?

In a related note, Log::Log4perl::Logger->DESTROY doesn't appear to do anything useful. It deletes things that will be automatically cleaned up in object destruction. Is that circular reference defense?

@mschilli
Owner

Should be fixed with

https://github.com/mschilli/log4perl/commit/d852b54d3537f24bf4a7fea76d8ab60c9e3885a3

Thanks for letting me know!

-- Mike

@mschilli mschilli closed this Jul 31, 2013
@jperkin jperkin pushed a commit to joyent/pkgsrc that referenced this issue Dec 9, 2013
rhaen Updated to 1.36
Changes:
1.36 (2012/02/11)
   *    (ms) [rt.cpan.org #74833] Reini Urban fixed "defined @array" for
             perl 5.16
   *    (ms) [rt.cpan.org #74836] Cope with Carp's questionable decision to
             add a trailing dot to its messages.

1.35 (2012/01/03)
   *    (ms) [rt.cpan.org #73462] Changed logwarn/logcluck/logcarp/error_warn
             to warn() unconditionally and send the message to log4perl which
             will log it only if the log level conditions are met.
   *    (ms) [rt.cpan.org #73598] Gerda Shank reported test suite problems
             with DBD::CSV-0.26. Bumped up to DBD::CSV-0.33 if installed.

1.34 (2011/11/04)
   *    (ms) InternalDebug now replaces all instances of INTERNAL_DEBUG,
             not just the first one.
   *    (ms) Added test case for get_logger() with a ref() on the actual
             object instead of on a static category. Updated docs.
   *    (ms) %d{e} in PatternLayout now returns epoch seconds
   *    (ms) [RT 72056] Messages blocked by an appender threshold are no
             longer passed on to the L4p::Appender::Buffer as undefined
             entries.

1.33 (2011/05/31)
   *    (ms) [RT 67132] Applied patch by Darin McBride to allow for
             empty syswrite messages in the file appender.
   *    (ms) [RT 68105] Fixed init-hash handling of subroutine references,
             reported by Frew Schmidt.
   *    (ms) Mike Schwern noticed confusing DESTROY calls to clean up loggers
             and appenders (http://stackoverflow.com/questions/5914088 and
             mschilli/log4perl#7), so I put on my
             hazmat suit and cleaned it up. Now perl's garbage collector takes
             care of disposing of logger and appender carcasses.
   *    (ms) Added Log::Log4perl->remove_logger($logger) to remove a logger
             from the system.

1.32 (2011/02/26)
   *    (ms) Fixed %T caller_depth with wrapper_register(), reported
             by David Christensen.
   *    (ms) [RT 63053] Fixed for qw() {} deprecated (Todd Rinaldo)
   *    (ms) [RT 62674] Fixed call to deprecated form of UNIVERSAL::can (Karen
             Etheridge).
   *    (ms) [RT 62896] Log::Log4perl::Appender::ScreenColoredLevels now
             inherits from Log::Log4perl::Appender::Screen and therefore
             supports the utf8 flag.
   *    (ms) [RT 64318] Andrew Sayers provided a better error message for
             "threshold needs to be uppercase".
   *    (ms) CharleyDixon fixed LOGWARN when :no_extra_logdie_message is
             in use to no longer exit().
3a91150
@jsonn jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Oct 11, 2014
rhaen Updated to 1.36
Changes:
1.36 (2012/02/11)
   *    (ms) [rt.cpan.org #74833] Reini Urban fixed "defined @array" for
             perl 5.16
   *    (ms) [rt.cpan.org #74836] Cope with Carp's questionable decision to
             add a trailing dot to its messages.

1.35 (2012/01/03)
   *    (ms) [rt.cpan.org #73462] Changed logwarn/logcluck/logcarp/error_warn
             to warn() unconditionally and send the message to log4perl which
             will log it only if the log level conditions are met.
   *    (ms) [rt.cpan.org #73598] Gerda Shank reported test suite problems
             with DBD::CSV-0.26. Bumped up to DBD::CSV-0.33 if installed.

1.34 (2011/11/04)
   *    (ms) InternalDebug now replaces all instances of INTERNAL_DEBUG,
             not just the first one.
   *    (ms) Added test case for get_logger() with a ref() on the actual
             object instead of on a static category. Updated docs.
   *    (ms) %d{e} in PatternLayout now returns epoch seconds
   *    (ms) [RT 72056] Messages blocked by an appender threshold are no
             longer passed on to the L4p::Appender::Buffer as undefined
             entries.

1.33 (2011/05/31)
   *    (ms) [RT 67132] Applied patch by Darin McBride to allow for
             empty syswrite messages in the file appender.
   *    (ms) [RT 68105] Fixed init-hash handling of subroutine references,
             reported by Frew Schmidt.
   *    (ms) Mike Schwern noticed confusing DESTROY calls to clean up loggers
             and appenders (http://stackoverflow.com/questions/5914088 and
             mschilli/log4perl#7), so I put on my
             hazmat suit and cleaned it up. Now perl's garbage collector takes
             care of disposing of logger and appender carcasses.
   *    (ms) Added Log::Log4perl->remove_logger($logger) to remove a logger
             from the system.

1.32 (2011/02/26)
   *    (ms) Fixed %T caller_depth with wrapper_register(), reported
             by David Christensen.
   *    (ms) [RT 63053] Fixed for qw() {} deprecated (Todd Rinaldo)
   *    (ms) [RT 62674] Fixed call to deprecated form of UNIVERSAL::can (Karen
             Etheridge).
   *    (ms) [RT 62896] Log::Log4perl::Appender::ScreenColoredLevels now
             inherits from Log::Log4perl::Appender::Screen and therefore
             supports the utf8 flag.
   *    (ms) [RT 64318] Andrew Sayers provided a better error message for
             "threshold needs to be uppercase".
   *    (ms) CharleyDixon fixed LOGWARN when :no_extra_logdie_message is
             in use to no longer exit().
336acd5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment