Mailing List Archive

Author: jm
Date: Mon May 9 00:50:39 2005
New Revision: 169246

URL: http://svn.apache.org/viewcvs?rev=169246&view=rev
Log:
bug 4305: remove use of Storable in spamd, due to being the possible cause of hangs on SMP systems

Modified:
spamassassin/trunk/INSTALL
spamassassin/trunk/lib/Mail/SpamAssassin.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
spamassassin/trunk/lib/Mail/SpamAssassin/NetSet.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm
spamassassin/trunk/spamd/spamd.raw

Modified: spamassassin/trunk/INSTALL
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/INSTALL?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/INSTALL (original)
+++ spamassassin/trunk/INSTALL Mon May 9 00:50:39 2005
@@ -209,25 +209,6 @@
Debian: apt-get install libhtml-parser-perl
Gentoo: emerge dev-perl/HTML-Parser

- - Storable (from CPAN)
-
- This is a required module if you use spamd and allow children to
- handle more than one client connection before quitting. Third party
- utilities may also require this module for the same functionality.
- Storable is used to shift configuration when a spamd process switches
- between users.
-
- If you plan to run SpamAssassin on a multiprocessor machine, or one
- with a hyperthreaded CPU like a Pentium 4, it is strongly recommended
- that you ensure version 2.12 (or newer) is installed. This fixes a
- bug that causes hangs under heavy load with that hardware
- configuration. (The fix might also be in version 2.10 already,
- see <http://bugzilla.spamassassin.org/show_bug.cgi?id=4010> for more
- information.)
-
- Debian: apt-get install libstorable-perl
- Gentoo: emerge dev-perl/Storable
-
Optional Modules
----------------


Modified: spamassassin/trunk/lib/Mail/SpamAssassin.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin.pm?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin.pm Mon May 9 00:50:39 2005
@@ -1670,27 +1670,6 @@

###########################################################################

-# private function to find out if the Storable function is available...
-sub _is_storable_available {
- my ($self) = @_;
-
- # Storable spews some warnings
- local $SIG{__DIE__};
-
- if (exists $self->{storable_available}) {
- }
- elsif (!eval { require Storable; }) {
- $self->{storable_available} = 0;
- dbg("generic: no Storable module found");
- }
- else {
- $self->{storable_available} = 1;
- dbg("generic: Storable module v".$Storable::VERSION." found");
- }
-
- return $self->{storable_available};
-}
-
=item $f->copy_config ( [ $source ], [ $dest ] )

Used for daemons to keep a persistent Mail::SpamAssassin object's
@@ -1712,90 +1691,36 @@
$spamtest->copy_config(\%conf_backup, undef) ||
die "config: error returned from copy_config!\n";

+Note that the contents of the associative arrays should be considered
+opaque by calling code.
+
=cut

sub copy_config {
- my($self, $source, $dest) = @_;
+ my ($self, $source, $dest) = @_;

# At least one of either source or dest needs to be a hash reference ...
unless ((defined $source && ref($source) eq 'HASH') ||
- (defined $dest && ref($dest) eq 'HASH')) {
+ (defined $dest && ref($dest) eq 'HASH'))
+ {
return 0;
}

- # We need the Storable module for this, so if it's not available,
- # return an error.
- return 0 if (!$self->_is_storable_available());
-
- # Set the other one to be the conf object
- $source ||= $self->{conf};
- $dest ||= $self->{conf};
-
- # if the destination sed_path_cache exists, destroy it and only copy
- # back what should be there...
- delete $dest->{sed_path_cache};
-
- # Copy the source array to the dest array
- while(my($k,$v) = each %{$source}) {
- # we know the main value doesn't need to get copied.
- # also ignore anything plugin related, since users can't change that,
- # and there are usually code references.
- next if ($k eq 'main' || $k =~ /plugin/ || $k eq 'registered_commands');
-
-
- my $i = ref($v);
-
- # Not a reference? Just copy the value over.
- if (!$i) {
- $dest->{$k} = $v;
- }
- elsif ($k =~ /^(internal|trusted)_networks$/) {
- # these are objects, but have a single hash array of interest
- # it may not exist though, so deal with it appropriately.
-
- # if it exists and is defined, copy it to the destination
- if ($v->{nets}) {
- # just copy the nets reference over ...
- $dest->{$k}->{nets} = Storable::dclone($v->{nets});
- }
- else {
- # this gets a little tricky...
- #
- # If $dest->{$k} doesn't exist, we're copying from the
- # config to a backup. So make a note that we want to delete
- # any configured nets by setting to undef.
- #
- # If $dest->{$k} does exist, we're copying back to the config
- # from the backup, so delete {nets}.
-
- if (exists $dest->{$k}) {
- delete $dest->{$k}->{nets};
- }
- else {
- $dest->{$k}->{nets} = undef;
- }
- }
- }
- elsif ($k eq 'scores') {
- # this is dealt with below, but we need to ignore it for now
- }
- elsif ($i eq 'SCALAR' || $i eq 'ARRAY' || $i eq 'HASH') {
- # IMPORTANT: DO THIS AFTER EVERYTHING ELSE!
- # If we don't do this at the end, any "special" object handling
- # will be screwed. See bugzilla ticket 3317 for more info.
-
- # Make a recursive copy of the reference.
- $dest->{$k} = Storable::dclone($v);
+ # let the Conf object itself do all the heavy lifting. It's better
+ # than having this class know all about that class' internals...
+ if (defined $source) {
+ dbg ("config: copying current conf from backup");
+ $self->{conf} = $source->{obj}->clone();
+ }
+ else {
+ dbg ("config: copying current conf to backup");
+ if ($dest->{obj}) {
+ # delete any existing copies first, to ensure that
+ # circular references are cleaned up
+ $dest->{obj}->finish();
}
-# else {
-# # throw a warning for debugging -- should never happen in normal usage
-# warn ">> $k, $i\n";
-# }
+ $dest->{obj} = $self->{conf}->clone();
}
-
- # deal with $conf->{scores}, it needs to be a reference into the scoreset
- # hash array dealy
- $dest->{scores} = $dest->{scoreset}->[$dest->{scoreset_current}];

return 1;
}

Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm Mon May 9 00:50:39 2005
@@ -128,6 +128,17 @@
$MISSING_REQUIRED_VALUE = -998;
$INVALID_VALUE = -999;

+# keys that should not be copied in ->clone()
+my @NON_COPIED_KEYS = qw(
+ main eval_plugins plugins_loaded registered_commands sed_path_cache parser
+ scoreset scores
+);
+
+# keys that should can be copied using a ->clone() method, in ->clone()
+my @CLONABLE_KEYS = qw(
+ internal_networks trusted_networks
+);
+
# set to "1" by the test suite code, to record regression tests
# $Mail::SpamAssassin::Conf::COLLECT_REGRESSION_TESTS = 1;

@@ -2978,6 +2989,58 @@
sub register_eval_rule {
my ($self, $pluginobj, $nameofsub) = @_;
$self->{eval_plugins}->{$nameofsub} = $pluginobj;
+}
+
+###########################################################################
+
+sub clone {
+ my ($self) = @_;
+ my $dest = Mail::SpamAssassin::Conf->new($self->{main});
+ my %done = ();
+
+ # special cases. first, skip anything that cannot be changed
+ # by users, and the stuff we take care of here
+ foreach my $var (@NON_COPIED_KEYS, @CLONABLE_KEYS) {
+ $done{$var} = undef;
+ }
+
+ foreach my $key (@CLONABLE_KEYS) {
+ $dest->{$key} = $self->{$key}->clone();
+ }
+
+ # scoresets
+ for my $i (0 .. 3) {
+ %{$dest->{scoreset}->[$i]} = %{$self->{scoreset}->[$i]};
+ }
+
+ # deal with $conf->{scores}, it needs to be a reference into the scoreset
+ # hash array dealy
+ $dest->{scores} = $dest->{scoreset}->[$dest->{scoreset_current}];
+
+ # ensure we don't copy the path cache from the master
+ delete $dest->{sed_path_cache};
+
+ # and now, copy over all the rest -- the less complex cases.
+ while(my($k,$v) = each %{$self}) {
+ next if exists $done{$k}; # we handled it above
+ my $i = ref($v);
+
+ # Not a reference, or a scalar? Just copy the value over.
+ if (!$i || $i eq 'SCALAR') {
+ $dest->{$k} = $v;
+ }
+ elsif ($i eq 'ARRAY') {
+ @{$dest->{$k}} = @{$v};
+ }
+ elsif ($i eq 'HASH') {
+ %{$dest->{$k}} = %{$v};
+ }
+ else {
+ # throw a warning for debugging -- should never happen in normal usage
+ warn "config: dup unknown type $k, $i\n";
+ }
+ }
+ return $dest;
}

###########################################################################

Modified: spamassassin/trunk/lib/Mail/SpamAssassin/NetSet.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin/NetSet.pm?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/NetSet.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/NetSet.pm Mon May 9 00:50:39 2005
@@ -102,6 +102,15 @@
0;
}

+sub clone {
+ my ($self) = @_;
+ my $dup = Mail::SpamAssassin::NetSet->new();
+ if (defined $self->{nets}) {
+ @{$dup->{nets}} = @{$self->{nets}};
+ }
+ return $dup;
+}
+
###########################################################################

1;

Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm Mon May 9 00:50:39 2005
@@ -41,16 +41,6 @@
HTML is used for an ever-increasing amount of email so this dependency
is unavoidable. Run "perldoc -q html" for additional information.',
},
-{
- 'module' => 'Storable',
- 'version' => '0.00', # 0.00 required, 2.12 is optional (below)
- 'desc' => 'This is a required module if you use spamd and allow user
- configurations to be used (ie: you don\'t use -x, -u, -q/--sql-config,
- -Q/--setuid-with-sql, --ldap-config, or --setuid-with-ldap). Third
- party utilities may also require this module for the same
- functionality. Storable is used to shift configuration when a spamd
- process switches between users.',
-},
);

my @OPTIONAL_MODULES = (

Modified: spamassassin/trunk/spamd/spamd.raw
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/spamd/spamd.raw?rev=169246&r1=169245&r2=169246&view=diff
==============================================================================
--- spamassassin/trunk/spamd/spamd.raw (original)
+++ spamassassin/trunk/spamd/spamd.raw Mon May 9 00:50:39 2005
@@ -688,10 +688,6 @@
$copy_config_p = 0;
}

-# If we need Storable, and it's not installed, alert now before we daemonize.
-die "spamd: required module Storable not found\n"
- if ($copy_config_p && !$spamtest->_is_storable_available());
-
## DAEMONIZE! ##

$opt{'daemonize'} and daemonize();
@@ -715,7 +711,7 @@
}

$spamtest->copy_config(undef, \%conf_backup) ||
- die "spamd: error returned from copy_config, no Storable module?\n";
+ die "spamd: error returned from copy_config\n";
}

# setup signal handlers before the kids since we may have to kill them...
@@ -915,6 +911,8 @@
{
# use a timeout! There are bugs in Storable on certain platforms
# that can cause spamd to hang -- see bug 3828 comment 154.
+ # we don't use Storable any more, but leave this in -- just
+ # in case.
my $oldalarm = 0;
eval {
local $SIG{ALRM} = sub { die "__alarm__\n" };
@@ -928,7 +926,7 @@
# (potentially), so let's restore back the saved version we
# had before.
$spamtest->copy_config(\%conf_backup, undef) ||
- die "error returned from copy_config, no Storable module?\n";
+ die "error returned from copy_config\n";
alarm $oldalarm;
};