Menu
▾
▴
log4perl-devel
|
From: Mike S. <m...@pe...> - 2004-05-29 08:08:58
|
Hi all,
here's a first stab at the circular references, patch attached. With
this in place, Tim's Mail appender example will work if you specify an
end block at the beginning to reset the exit code *after* the mail
appender gets destroyed:
END { $? = 255; }
use Log::Log4perl qw(get_logger :levels);
Log::Log4perl->init(\q{
log4perl.logger = DEBUG, Mailer
log4perl.appender.Mailer = Log::Dispatch::Email::MailSend
log4perl.appender.Mailer.to = em...@so...
log4perl.appender.Mailer.subject = mysubject
log4perl.appender.Mailer.layout = Log::Log4perl::Layout::PatternLayout
log4perl.appender.Mailer.layout.ConversionPattern = %m%n
});
my $logger = get_logger("foo");
$logger->logdie( "boo" );
--
-- Mike
Mike Schilli
m...@pe...
Index: lib/Log/Log4perl.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl.pm,v
retrieving revision 1.164
diff -a -u -r1.164 Log4perl.pm
--- lib/Log/Log4perl.pm 28 May 2004 23:41:54 -0000 1.164
+++ lib/Log/Log4perl.pm 29 May 2004 07:49:20 -0000
@@ -2,6 +2,9 @@
package Log::Log4perl;
##################################################
+ # Have this first to execute last
+END { Log::Log4perl::Logger::cleanup(); }
+
use 5.006;
use strict;
use warnings;
Index: lib/Log/Log4perl/Appender.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl/Appender.pm,v
retrieving revision 1.35
diff -a -u -r1.35 Appender.pm
--- lib/Log/Log4perl/Appender.pm 26 Apr 2004 02:41:10 -0000 1.35
+++ lib/Log/Log4perl/Appender.pm 29 May 2004 07:49:21 -0000
@@ -260,7 +260,15 @@
##################################################
sub DESTROY {
##################################################
- # just there because of AUTOLOAD
+ # use Data::Dumper;
+
+ # warn "Destroying appender $_[0]";
+ # warn "app=", Dumper($_[0]), "-";
+
+ foreach my $key (keys %{$_[0]}) {
+ # print "deleting $key\n";
+ delete $_[0]->{$key};
+ }
}
1;
Index: lib/Log/Log4perl/Logger.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl/Logger.pm,v
retrieving revision 1.60
diff -a -u -r1.60 Logger.pm
--- lib/Log/Log4perl/Logger.pm 9 Oct 2003 17:07:11 -0000 1.60
+++ lib/Log/Log4perl/Logger.pm 29 May 2004 07:49:22 -0000
@@ -23,6 +23,56 @@
__PACKAGE__->reset();
##################################################
+sub cleanup {
+##################################################
+ # warn "Logger cleanup";
+
+ # Delete all loggers
+ foreach my $loggername (keys %$LOGGERS_BY_NAME){
+ # warn "Logger delete: $loggername";
+ $LOGGERS_BY_NAME->{$loggername}->DESTROY();
+ delete $LOGGERS_BY_NAME->{$loggername};
+ }
+
+ # Delete the root logger
+ #$ROOT_LOGGER->DESTROY() if defined $ROOT_LOGGER;
+ undef $ROOT_LOGGER;
+ $LOGGERS_BY_NAME = ();
+
+ # warn scalar(keys %APPENDER_BY_NAME), " appenders leftover";
+
+ # Delete all appenders
+ foreach my $appendername (keys %APPENDER_BY_NAME){
+ if (exists $APPENDER_BY_NAME{$appendername} &&
+ exists $APPENDER_BY_NAME{$appendername}->{appender}) {
+ # warn "Appender delete: $appendername ",
+ # "($APPENDER_BY_NAME{$appendername}->{appender})";
+ # Destroy L4p::Appender::Whatever
+# $APPENDER_BY_NAME{$appendername}->{appender}->DESTROY() if
+#
$APPENDER_BY_NAME{$appendername}->{appender}->can("DESTROY");
+ # Destroy L4p::Appender
+ $APPENDER_BY_NAME{$appendername}->DESTROY();
+ delete $APPENDER_BY_NAME{$appendername}->{appender};
+ delete $APPENDER_BY_NAME{$appendername};
+ }
+ }
+ %APPENDER_BY_NAME = ();
+}
+
+# use Data::Dumper;
+
+##################################################
+sub DESTROY {
+##################################################
+ # warn "Destroying logger $_[0]";
+ # print Dumper($_[0]);
+ for(keys %{$_[0]}) {
+ # print "Deleting key $_\n";
+ delete $_[0]->{$_};
+ }
+}
+
+##################################################
sub reset {
##################################################
$ROOT_LOGGER = __PACKAGE__->_new("", $DEBUG);
|
|
From: Kevin G. <ke...@go...> - 2004-05-30 06:14:11
|
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Mike, thanks for doing all that work, fantastic stuff except for two
points: I think the circular reference thing is a red herring, although
I agree with the approach in your patch, and you need a local($?) in
your END block:
First, did you actually track down the circular reference the Logger.pm
mentioned? Maybe I'm being dense (probably) but I don't see it in the
dump you sent. I checked out a basic $logger under use
Test::Memory::Cycle and didn't see any circular references.
Are you sure Tim's exit value problem is related to circular references?
~ It seems to me that since we're using package variables to store our
loggers, they're not going to be destroyed until the Global Destruction
phase of the script, the reference in the package symbol table won't go
away until then. That behavior seems correct to me.
So the Mail appender's DESTROY is being called during the Global
Destruction phase and overwriting $?. Maybe what we need to do is
"local($?)" before then--if we can localize $? in the call stack above
where the flush() happens then the correct value will be restored after
the flush completes, right?
To do that we need to do the flushing before the GDP kicks in, so your
END block in Log4perl.pm and your cleanup() methods are the right idea.
~ However, your (Mike's) example sets $? in that BEGIN block, but doesn't
help Tim's code, his $? isn't from a BEGIN block. However, if you change
END { Log::Log4perl::Logger::cleanup(); }
to
END { local($?); Log::Log4perl::Logger::cleanup(); }
Then Tim's code does finish with the proper exit strategy.
How does that sound? Let me know if you don't follow me, in fact feel
free to phone me if I'm not clear, I'm around all weekend (during normal
waking hours, that is).
- --
Happy Trails. . .
Kevin M. Goess
(and Anne and Frank)
904 Carmel Ave.
Albany, CA 94706
(510)525-5217
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org
iD8DBQFAuXpo4g4/Tl71vUkRAu2TAKCsPxT+OYfia+YGTjA901ZVEiZBjwCgt/rc
p6wF9m73KF/VwP+oX27dIEk=
=Mdad
-----END PGP SIGNATURE-----
|
|
From: Mike S. <m...@pe...> - 2004-06-01 07:03:34
|
Kevin Goess wrote on 5/29/2004, 11:08 PM:
> First, did you actually track down the circular reference the Logger.pm
> mentioned? Maybe I'm being dense (probably) but I don't see it in the
> dump you sent. I checked out a basic $logger under use
> Test::Memory::Cycle and didn't see any circular references.
Hmm, you could be on to someting: Maybe the reason why the loggers
aren't cleaned up correctly isn't circular references, it's simply that
there's still references lurking around.
They can be held by package variables like
$Log::Log4perl::Logger::ROOT_LOGGER and also by non-lexical variables
which have obtained a logger instance in the main program:
no strict;
$logger = get_logger("Foo"); # Not a "my" variable
On the other hand, if there's no circular reference, why does something
like the following leak memory?
use Log::Log4perl qw(get_logger);
while(1) {
Log::Log4perl->init(\ q{
log4perl.logger = DEBUG, A1
log4perl.appender.A1 = Log::Log4perl::Appender::Screen
log4perl.appender.A1.layout = SimpleLayout
});
$logger = get_logger("foo");
}
> So the Mail appender's DESTROY is being called during the Global
> Destruction phase and overwriting $?. Maybe what we need to do is
> "local($?)" before then--if we can localize $? in the call stack above
> where the flush() happens then the correct value will be restored after
> the flush completes, right?
I'm not sure if you can localize $? while the GDP happens.
> ~ However, your (Mike's) example sets $? in that BEGIN block, but doesn't
> help Tim's code, his $? isn't from a BEGIN block.
Hmm, I didn't use a BEGIN block -- not sure I'm following.
> However, if you change
> END { Log::Log4perl::Logger::cleanup(); }
> to
> END { local($?); Log::Log4perl::Logger::cleanup(); }
> Then Tim's code does finish with the proper exit strategy.
Didn't my patch work with Tim's code? I added END { $? = 255 } to Tim's
snippet and it exited correctly with 255.
Guess we need to talk more about this, unfortunately, it's too late
today for non-nocturnals ... :)
--
-- Mike
Mike Schilli
m...@pe...
|
|
From: Kevin G. <ke...@go...> - 2004-06-01 17:55:39
|
Ignoring for now the question of memory leaks
Ignorin
> Didn't my patch work with Tim's code? I added END { $? = 255 } to Tim's
> snippet and it exited correctly with 255.
But Tim shouldn't have to add END blocks to his code. I believe the
problem can be summarized by this code:
------------------------
#!/usr/bin/perl
sub cleanup {
`/bin/true`; #some shell call
}
die "arrgh";
END{cleanup()}
------------------------
If you run that like this
./test.pl && echo "program ran sucessfully"
you should *not* see the success message, because it *died*. It looks
to me like the $? from cleanup() is overriding the result of the die().
Tim is calling logdie() and getting a successful exit code from his
script because deep in the guts of Mail::Mailer there's shell code
calling the mailer program that's succeeding. That success value seems
to be what his script is exiting with. Log4perl shouldn't be changing
exit values to 'success' just because some appender has a successful
shell function.
The test script runs as expected if you change the END block to
END{local($?); cleanup()}
In that case, Tim's script also runs as expected, without the addition
of an additional END block.
So we're not localzing during the GDP, we're making sure all the cleanup
happens before the GDP.
How does that sound? I feel like I'm not being clear, and I don't know
why, but I do think this is the right track.
--
Happy Trails . . .
Kevin M. Goess
(and Anne and Frank)
904 Carmel Ave.
Albany, CA 94706
(510) 525-5217
|
|
From: Mike S. <m...@pe...> - 2004-06-05 17:19:17
|
Kevin Goess wrote on 6/1/2004, 10:55 AM:
> The test script runs as expected if you change the END block to
>
> END{local($?); cleanup()}
>
> In that case, Tim's script also runs as expected, without the addition
> of an additional END block.
Sorry for the long wait, Kevin, you are 100% correct, what was I thinking!
I've added your local() fix to the patch and the dietest.pl script as in
use Log::Log4perl qw(get_logger :levels);
Log::Log4perl->init(\q{
log4perl.logger = DEBUG, Mailer
log4perl.appender.Mailer = Log::Dispatch::Email::MailSend
log4perl.appender.Mailer.to = mschilli@localhost
log4perl.appender.Mailer.subject = mysubject
log4perl.appender.Mailer.layout =
Log::Log4perl::Layout::PatternLayout
log4perl.appender.Mailer.layout.ConversionPattern = %m%n
});
my $logger = get_logger("foo");
$logger->logdie( "boo" );
now works fine with an exit value of 255. Yay! :)
Regarding the mem leak, the 'warn' statements in the patch now show that
when running dietest.pl above, one appender is still left over for
destruction in the GDP (although it doesn't matter anymore for $?,
because its 'meat' is already gone at this point) -- do you have any
ideas on how we can track down where it's still referenced from?
--
-- Mike
Mike Schilli
m...@pe...
Index: lib/Log/Log4perl.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl.pm,v
retrieving revision 1.164
diff -a -u -r1.164 Log4perl.pm
--- lib/Log/Log4perl.pm 28 May 2004 23:41:54 -0000 1.164
+++ lib/Log/Log4perl.pm 5 Jun 2004 17:10:02 -0000
@@ -2,6 +2,9 @@
package Log::Log4perl;
##################################################
+ # Have this first to execute last
+END { local($?); Log::Log4perl::Logger::cleanup(); }
+
use 5.006;
use strict;
use warnings;
Index: lib/Log/Log4perl/Appender.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl/Appender.pm,v
retrieving revision 1.35
diff -a -u -r1.35 Appender.pm
--- lib/Log/Log4perl/Appender.pm 26 Apr 2004 02:41:10 -0000 1.35
+++ lib/Log/Log4perl/Appender.pm 5 Jun 2004 17:10:03 -0000
@@ -260,7 +260,15 @@
##################################################
sub DESTROY {
##################################################
- # just there because of AUTOLOAD
+ # use Data::Dumper;
+
+ warn "Destroying appender $_[0]";
+ # warn "app=", Dumper($_[0]), "-";
+
+ foreach my $key (keys %{$_[0]}) {
+ # print "deleting $key\n";
+ delete $_[0]->{$key};
+ }
}
1;
Index: lib/Log/Log4perl/Logger.pm
===================================================================
RCS file: /cvsroot/log4perl/Log-Log4perl/lib/Log/Log4perl/Logger.pm,v
retrieving revision 1.60
diff -a -u -r1.60 Logger.pm
--- lib/Log/Log4perl/Logger.pm 9 Oct 2003 17:07:11 -0000 1.60
+++ lib/Log/Log4perl/Logger.pm 5 Jun 2004 17:10:04 -0000
@@ -23,6 +23,45 @@
__PACKAGE__->reset();
##################################################
+sub cleanup {
+##################################################
+ # warn "Logger cleanup";
+
+ # Delete all loggers
+ foreach my $loggername (keys %$LOGGERS_BY_NAME){
+ # warn "Logger delete: $loggername";
+ $LOGGERS_BY_NAME->{$loggername}->DESTROY();
+ delete $LOGGERS_BY_NAME->{$loggername};
+ }
+
+ # Delete the root logger
+ undef $ROOT_LOGGER;
+ $LOGGERS_BY_NAME = ();
+
+ # Delete all appenders
+ foreach my $appendername (keys %APPENDER_BY_NAME){
+ if (exists $APPENDER_BY_NAME{$appendername} &&
+ exists $APPENDER_BY_NAME{$appendername}->{appender}) {
+ # Destroy L4p::Appender
+ $APPENDER_BY_NAME{$appendername}->DESTROY();
+ delete $APPENDER_BY_NAME{$appendername}->{appender};
+ delete $APPENDER_BY_NAME{$appendername};
+ }
+ }
+ %APPENDER_BY_NAME = ();
+}
+
+##################################################
+sub DESTROY {
+##################################################
+ warn "Destroying logger $_[0]";
+
+ for(keys %{$_[0]}) {
+ delete $_[0]->{$_};
+ }
+}
+
+##################################################
sub reset {
##################################################
$ROOT_LOGGER = __PACKAGE__->_new("", $DEBUG);
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X