Bug 5131 - RFE: graceful config re-reading in spamd
Summary: RFE: graceful config re-reading in spamd
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: Other other
: P5 enhancement
Target Milestone: Future
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
: 4914 (view as bug list)
Depends on:
Blocks: 4560
  Show dependency tree
 
Reported: 2006-10-17 02:53 UTC by Justin Mason
Modified: 2010-04-21 20:42 UTC (History)
2 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Mason 2006-10-17 02:53:50 UTC 
It should be possible for spamd to reload the configuration "gracefully" -- ie.
in the same process, without closing the accept socket or losing the listen
queue of incoming connections, somehow.  (In fact I think some of the commercial
derivatives support it. ;)

I would suggest this algorithm:

in spamd parent, when SIGHUP is received:
- a flag is set, and signal handler returns

in the spamd mainline, at the "if ( defined $got_sighup ) {" check:
- if that flag is set:
- the Mail::SpamAssassin object ->finish() method is called
- a new Mail::SpamAssassin object is created instead
- all child PIDs are signalled with a new sig indicating "exit asap"

for each child PID:
- if they're busy scanning a mail, it continues to completion, then exits
- if they're idle, they exit immediately before accepting another conn

Benefits:

- this should be faster than the existing method of killing all spamd processes

- issues regarding "old" spamd processes holding onto the listen socket,
resulting in EADDRINUSE, will not happen

- since it allows existing scan processes to complete, instead of killing them,
there's less possibility of cases where mail will be delivered unscanned


Downsides:

- failure to clear up circular refs or generated methods will become more
obvious, since those will now create ongoing memory leaks over multiple
restarts.  mind you, t/recreate.t should be finding them nowadays anyway.
Comment 1 Justin Mason 2006-10-17 02:54:28 UTC 
optimistic aiming at 3.2.0
Comment 2 Tom Schulz 2006-10-17 06:21:15 UTC 
See also Bug 4914 and Bug 4774 comment 2.
Comment 3 Justin Mason 2006-10-17 06:29:49 UTC 
*** Bug 4914 has been marked as a duplicate of this bug. ***
Comment 4 Justin Mason 2006-10-17 06:31:05 UTC 
thanks Tom -- they are indeed dups.  I'll keep this GSoC description from bug
4914...

'This was a suggested idea for the Google Summer of Code 2006;
I'm adding it to the bugzilla for future use, and in case anyone feels
like implementing it.

Subject ID: spamassassin-better-reload
Keywords: reload, spamd, sighup, restart, perl
Description: http://issues.apache.org/SpamAssassin/show_bug.cgi?id=5131 : we
currently have a very heavyweight configuration-rereading system where the
entire process restarts. This is too heavyweight, and can be improved.
Possible Mentors: Justin Mason (jm at jmason.org)'
Comment 5 Bob Van Zant 2006-10-17 10:36:35 UTC 
Simply purging the current Mail::SpamAssassin object and creating a new one to load a new 
configuration set into doesn't work. Nearly all rules are stored in some global area. This means that 
when you kill the Mail::SpamAssassin object you don't actually kill all of the loaded rules.

Now look at how rules are executed in, for instance, PerMsgStatus.pm::do_head_tests(). We loop over 
the static data in Mail::SpamAssassin::PerMsgStatus::_head_tests_*.

So, imagine for a moment what happens when you remove rules from a rules file between HUPs. That's 
right: the rules continue to be run because nothing actually purges all of the in-memory rules.

A valid next step in trying to solve this is to head over to 
Mail::SpamAssassin::PerMsgStatus::_head_tests_ and start deleting all of the loaded rules. Do this for 
head tests, body tests, raw body tests etc. Great, the hack works. Wrong. There are some plugins that 
come with SA that store their data in some made-up global namespace. Now you have special cases to 
clean up. Ugg.

Why not fix this correctly by cleaning up the config namespace such that all config data is stored in one 
nice container. Purge the container: purge all of the rules.

HUP is the easy part. It's like a 20-line patch at most. This isn't the problem though. The problem is 
finding and refactoring all of the different rule plugins that store their data in ruletype-specific places 
and changing them to be stored in a nice container that is a member of a Mail::SpamAssassin instance.
Comment 6 Justin Mason 2006-10-17 10:49:00 UTC 
actually, Bob, the deletion of stuff from _do_head_tests etc. is already done in
3.2.0, as part of Mail::SA::finish().

plugins that are stuffing things into the global namespace, instead of using the
'conf' object that's passed around for this purpose -- that's a bug, and they
will simply need to be fixed.
Comment 7 Michael Parker 2006-10-17 11:55:18 UTC 
The Check plugin branch moves finish_tests to be a plugin API call, so different
plugins can be responsible for cleaning up their own mess.
Comment 8 Theo Van Dinter 2006-12-05 10:49:25 UTC 
no code, punting.
Comment 9 Justin Mason 2010-01-27 02:19:39 UTC 
moving most remaining 3.3.0 bugs to 3.3.1 milestone
Comment 10 Justin Mason 2010-01-27 03:14:53 UTC 
these were not supposed to go to security!  moving back out of that component, but now probably to the wrong components :( , but better than nothing.
Comment 11 Justin Mason 2010-01-27 03:15:21 UTC 
these were not supposed to go to security!  moving back out of that component, but now probably to the wrong components :( , but better than nothing.
Comment 12 Justin Mason 2010-01-27 03:16:01 UTC 
reassigning, too
Comment 13 Justin Mason 2010-03-23 16:32:52 UTC 
moving all open 3.3.1 bugs to 3.3.2
Comment 14 Karsten Bräckelmann 2010-03-23 17:42:19 UTC 
Moving back off of Security, which got changed by accident during the mass Target Milestone move.