I'll have to defer to someone else as most of that was way over my head but
I do know we have this ticket outstanding:
https://issues.apache.org/jira/browse/TC-74
and also, this PR was merged a while back regarding $SIG{CHLD} -
https://github.com/apache/incubator-trafficcontrol/pull/155
I'm not really sure if it's a good or bad idea to wrap every call to
$self->mail(...);
as you suggested. maybe others have thoughts on that.
preferably, it would be nice to get TC-74 done and remove our dependency on
an outdated library.
jeremy
On Tue, Feb 7, 2017 at 4:27 AM, Oren Shemesh <[email protected]> wrote:
> Hi,
>
> I'm looking for someone to form an opinion about a problem I believe I have
> found, and a fix.
>
> The problem manifests itself as an error when TO attempts to send an
> E-mail.
>
> The details:
> When traffic ops runs, it daemonizes itself, and as part of that, it does
> $SIG{CHLD} = 'IGNORE'; :
>
> https://github.com/apache/incubator-trafficcontrol/blob/
> master/traffic_ops/app/lib/MojoPlugins/Daemonize.pm#L28
>
> But, when sending a mail, TO uses Mojolicous::Plugin::Mail, which in turn
> is a wrapper for the standard (And Obsolete) perl module MIME::Lite.
>
> MIME::Lite sends a mail by running the following piece of code (Snippet
> from app/local/lib/perl5/MIME/Lite.pm):
>
> my $pid = open SENDMAIL, "|-";
> defined($pid) or die "open of pipe failed: $!\n";
> if ( !$pid ) { ### child
> exec(@cmd) or die "can't exec $p{Sendmail}: $!\n";
> ### NOTREACHED
> } else { ### parent
> $self->print( \*SENDMAIL );
> close SENDMAIL || die "error closing $p{Sendmail}: $! (exit
> $?)\n";
> $return = 1;
> }
>
> The problem with this code is that assumes that the SIGCHLD handler is left
> at it's DEFAULT handling, which means that when a child process terminates,
> the parent process can reap it during the 'close' statement.
> Traffic Ops runs with the SIGCHLD handler set to IGNORE, which means that
> when a child process dies, the parent process forgets about it immediately.
> So, if sendmail runs fast enough and finishes before the 'close' command is
> executed, the close will fail to find the child process, and the error
> propagates all the way to the calling code, looking something like this:
>
> Traffic Ops fatal error occurred while processing your request.
> Error at line 2732 ( close SENDMAIL || die "error closing $p{Sendmail}: $!
> (exit $?)\n";)
> error closing /usr/lib/sendmail: No child processes (exit -1)
>
> It should be noted that we are running TO on single-core machines for
> development, which means that the code is much more likely to fail in this
> manner (In machines with more cores, the parent process usually does not
> get suspended when sendmail is spawned, hence it reaches the 'close'
> command immediately and waits for sendmail to finish properly).
>
> I am not sure why TO needs to completely ignore SIGCHLD, and I am not sure
> if it is reasonable to ask from a module such as MIME::Lite to work when
> SIGCHLD is set to IGNORE.
> However, MIME::Lite is not being maintained any more (The owner recommends
> using better modules, see this
> <http://search.cpan.org/~rjbs/MIME-Lite-3.030/lib/MIME/Lite.pm#WAIT!>),
> and
> the latest Mojolicous::Plugin::Mail version still uses MIME::Lite.
> (I guess Mojolicous is not very actively maintained these days).
> Which means that until we switch to the experimental, Angular-based,
> Traffic Ops, we are stuck with the current MIME::Lite code.
>
> Maybe a good fix for this issue is to not set SIGCHLD to IGNORE while
> daemonizing. However, this means that every time we create a new process,
> we must waitpid() for it, otherwise we will accumulate zombies.
> I guess it is not very practical to ensure this across the entire code base
> of TO (And, as we have seen, all Perl modules that it uses).
>
> So the low-footprint fix I want to make is to wrap every call to
> $self->mail(...) done in our Email.pm
> <https://github.com/apache/incubator-trafficcontrol/blob/
> master/traffic_ops/app/lib/MojoPlugins/Email.pm>
> code, like this:
>
> $SIG{CHLD} = 'DEFAULT';
> $self->mail(...);
> $SIG{CHLD} = 'IGNORE';
>
> I tested this, and it fixes the problem.
>
> Thoughts ?
>
> I plan to create a JIRA and a PR, but I wanted to get someone's more expert
> view on this before I go on.
> Being a bit of a novice, maybe I am missing some key thing here.
>
> Thanks ,Oren.
> --
>
> *Oren Shemesh*
> Qwilt | Work: +972-72-2221637| Mobile: +972-50-2281168 | [email protected]
> <[email protected]>
>
