Thanks Jeremy. We did merge that PR not too long ago to fix one issue, but it looks like it created another. I agree that it would be better to use a different library.
On Tue, Feb 7, 2017 at 1:29 PM, Jeremy Mitchell <[email protected]> wrote: > 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]> > > >
