https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6942
Bug ID: 6942
Summary: Redis bayes storage module fixes and updates
Product: Spamassassin
Version: SVN Trunk (Latest Devel Version)
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: P2
Component: Plugins
Assignee: dev@spamassassin.apache.org
Reporter: mark.marti...@ijs.si
Spent a couple of days working on the Henrik Krohns' Redis storage backend
for Bayes (introduced by Bug 6879), fixing some of its problems and then
contributing some improvements. Testing was under Redis 2.6.13, perl 5.18.0,
on FreeBSD.
The problems first, two of them. The first was an inconsistent bookkeeping
of the $self->{is_officially_open} state, as revealed by debug logging.
The tie_db_* routines forgot to turn it on when $self->{is_really_open}
was already on. As a consequence some opportunistic calls reported
they cannot run because the backend is not open.
The second one looked quite mysterious: bayes seemed to work most of
the time, but every now and then reported either that number of spam
and ham message in the database was null, or the sub _unpack_token()
sometimes reported "bayes: unknown token format". Examining these
'unknown' tokens revealed that unrelated replies were treated as
tokens, e.g. the value of the "v:NHAM" key ended up as a value of
a token.
Turns out the cause is a re-use of cloned parent process socket by
a child process to connect to a Redis server. When several child
processes were busy at the same time, protocol got mixed up, resulting
in mixed up or missing results. When the load was light, everything
appeared to work normally.
At first I did a quick workaround, letting the module check its
process ID, comparing it to a PID under which the connection to
a Redis server was created, and re-establishing a session on a change.
This worked, but is somehow unclean, as the parent's redis sesssion
should really be closed by the master process and it should never
be propagated (through fork) to child processes.
Turns out the plugin callbacks did not offer any callback *before*
forkoff. Moreover, the existing "spamd_child_init" plugin did not
propagate through BayesStore to backend modules, so it could not
be used either.
As the opportunity of a major version bump is just right, I introduced
a new plugin callback "prefork_init", which spamd and amavisd and
mass-check now call before spawning child processes. The BayesStore
now propagates both the "prefork_init" and the "spamd_child_init"
to bayes storage modules which implement these optional methods.
The Redis backend module now closes its redis session in a master
process on "prefork_init", and (just in case) it also dismisses
existing session in "spamd_child_init", is the session happens
to still be open (e.g. some third party master process not yet
aware of the new "prefork_init" call).
While fiddling with the code, a couple of improvements were made:
- introduced new settings bayes_token_ttl and bayes_seen_ttl
instead of repurposing bayes_expiry_max_db_size;
- avoided some unnecessary data copying;
- added a "b_learn" timing report entry;
- added some checks and debug entries which helped me debug
the problems encountered;
- updated some comments
Seems to work very well and fast now!
A big thanks to Henrik for making this available!
--
You are receiving this mail because:
You are the assignee for the bug.
