pre-connection hook does not work as they are not loaded in qpsmtpd-forkserver

[francispintos]

The code to load plugins is commented out at line 196 qpsmtpd-forkserver, no hooks will be registered in the parent process. Since pre-connection hook is run before fork, all pre-connection plugins do not work. This is the case for version 0.96 and version 1.0.0.

[unnilennium]

sounds duplicate of issue #288

here is a fix

https://viewvc.koozali.org/smeserver/rpms/qpsmtpd/sme10/qpsmtpd-0.96-bz10387-load_plugins-on-start.patch?revision=1.1&view=markup

and for reference

https://bugs.koozali.org/show_bug.cgi?id=10387

[francispintos]

Thank you for the cross reference. I already did the fix temporarily in the code by enabling the loading of plugins. I am reporting this issue hoping to get a fix in the main line of code so that we don't have to apply our own patch every time there is a release.

[msimerson]

I can't remember the details, it has been too long, but I do recall that if that line is uncommented, then in some circumstance, plug-in hooks will be registered twice. Which is why it was commented out.

IMO, there were too many ways to run QPSMTPD. There is probably only one or two still in active use. I'd entertain a PR that simplified QP to just one run model, and assuring a single code path to loading plug-ins.

[francispintos]

For now, this issue only happens in the forkserver version. I am monitoring to see if my un-comment out load_plugins causes any problems. I am on 0.96 BTW.

[unnilennium]

we are running forkserver at Koozali SME Server and with the line commented the plugins are never loaded on start of the service. It needs a reload, as described in other issue, to have them loaded which leaves all servers after a boot without the plugins. this is more systematic than "in some circumstances". The result of commenting out sounds like a regression in an attempt to workaround less frequent situation.

Out of topic: Have you an idea of which way of running you plan to keep? thanks

[msimerson]

There are no plans for QP.

[abh]

we are running forkserver at Koozali SME Server and with the line commented the plugins are never loaded on start of the service. It needs a reload, as described in other issue, to have them loaded which leaves all servers after a boot without the plugins. this is more systematic than "in some circumstances".

Without remembering how any of this works, that sounds like maybe the plugins need to be loaded after the server forks, but where it's commented out is before the fork. (Or alternately it was commented out because some plugins are unhappy about being loaded in the main process and reloaded later after a fork).

(and again, I haven't contributed for more than 10 years so I don't know at all what I'm talking about here).

[unnilennium]

ok for the idea of having it loading just after the fork. however, from my understanding the following code is the only other one loading plugins there. and plugins need to be loaded somewhere.

$SIG{HUP} = sub {
    $qpsmtpd = Qpsmtpd::TcpServer->new('restart' => 1);
    $qpsmtpd->load_plugins;
    $qpsmtpd->spool_dir;
    $qpsmtpd->size_threshold;
};

with forkserver at koozali this is working with the line uncommented since the fix has been pushed for almost a year. before that plugins where not loaded as described. no error detected since with plugins. I would be happy to update if you see a better approach.

[francispintos]

I have uncommented the load_plugins call before fork for 1 week or so and did not see any problem.

As far as I can tell, plugin is suppose to handle being reloaded more than once, since all plugins gets reloaded when a SIGHUP is received.

[wornet-aer]

I am currently testing the 1.00 release version of qpsmtpd from github and trying to build a clean and fresh Debian/Ubuntu package, which we can then contribute to the offical package repos.

With the commented $qpsmtpd->load_plugins; the behaviour of qpsmtpd is very different than in 0.94 and breaks several plugins, which use the pre-connection hook when using the qpsmtpd-forkserver. For me this looks really bad.

If I had to choose between loading plugins twice or never, I'd choose twice because never is even worse.

Is there an old issue for this or any other information on how to reproduce this 'plugins are loaded twice' thing? So we can debug it more deeply and try to find a non-breaking solution for this.

[msimerson]

If I had to choose between loading plugins twice or never, I'd choose twice because never is even worse.

I think you say that because you aren't aware of the consequences of loading them twice. Loading plugins twice sometimes breaks things. Is it better for failures to be seemingly random or deterministic?

Going from memory: you'd likely have to go back to the qpsmtpd email archives to find information on the twice issue. From my recollections, more than a few very good programmers wandered into that thicket. If there were an easy solution, it managed to evade them all. Hence my suggestion above: ditch the alternate run models. Alternate run models were compromises born of a time when machines were starved for resources, especially RAM. The code can be made more robust, easier to use, and easier to set up by ditching all but one run model.

[wornet-aer]

Thanks for your detailed reply, Matt! :)

Ofcourse there is always another perspective on the topic and I do get your point, that it's also very bad, to have plugins loaded twice. The only email archive entry I've found its this one: https://www.mail-archive.com/qpsmtpd@perl.org/msg01789.html

Since we are heavily using the forkserver run model, I started some testing and research.

1. If you double the $qpsmtpd->load_plugins; in line 196 of qpsmtpd-forkserver, the plugins will NOT get loaded twice.
2. When looking at the HUP-Signal handler in the forkserver, it does also execute the load_plugins. So if this function were loading plugins multiple times, it must produce several plugin instances after each HUP signal. But this is NOT the case.
3. Looking at the Qpsmtpd.pm file where the load_plugins is done: It uses the _load_plugins method, which checks in line 200++ if the plugin has been already loaded. Which means, that you can execute the load_plugins any number of times, without getting duplicated plugin instances. The only consequence is, that the plugins may be re-loaded multiple times as it's done on the HUP signal. This should not produce any harmful outcomes.

So from my perspective, nothing breaks, if we were removing the comment from line 196 in qpsmtpd-forkserver again. @francispintos and @unnilennium have also already runnig their code WITH this line active and didn't face any issues.

Regarding the different run models, I would suggest to discuss this in a separate issue. I'd personally prefer using the forkserver, but I do understand that it's more code to maintain and things would get easier, if we were focusing on one single run model instead.

Cheers Andreas

[msimerson]

There's an entire thread that details the problems with loading plug-ins twice, and it seemed to revolve around CPAN published packages breaking when loading a second time. I suspect that it was a combination of such packages and a different run model (there were six or seven).

Go ahead and comment that line and I'll merge the PR, as it's been tested and used with forkserver, the only run model known in active use. I've started a PR to exorcise all the other other run models. Maybe I'll find time to finish it this year.