From eeb8d69bcd15002f806bfeed29a20c14e8aa4684 Mon Sep 17 00:00:00 2001 From: Hu Yin Date: Mon, 6 Feb 2023 11:33:54 -0500 Subject: [PATCH 1/3] Run cleanup handers first, then post processing Currently some post processing hooks are checks like memory check, if a worker hits a memory limit, it will try to recycle the worker. In that case, any post processing cleanup handling code wouldn't have a chance to run. This is to fix the execution order. --- lib/Plack/Handler/FCGI.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Plack/Handler/FCGI.pm b/lib/Plack/Handler/FCGI.pm index 754a294d..ae68ce9b 100644 --- a/lib/Plack/Handler/FCGI.pm +++ b/lib/Plack/Handler/FCGI.pm @@ -162,8 +162,6 @@ sub run { # the request is done $request->Finish; - $proc_manager && $proc_manager->pm_post_dispatch(); - # When the fcgi-manager exits it sends a TERM signal to the workers. # However, if we're busy processing the cleanup handlers, testing # shows that the worker doesn't actually exit in that case. @@ -176,6 +174,8 @@ sub run { } } + $proc_manager && $proc_manager->pm_post_dispatch(); + if ($proc_manager && $env->{'psgix.harakiri.commit'}) { $proc_manager->pm_exit("safe exit with harakiri"); } From 1d31610de5f5f59231bcf37362af7b5f5a7cea9a Mon Sep 17 00:00:00 2001 From: Hu Yin Date: Tue, 14 Feb 2023 15:21:45 -0500 Subject: [PATCH 2/3] Test cleanup handler and Proc Manager Term signal handling Manually set n_process=0 and call hanlding_init to avoid manager forking --- cpanfile | 1 + t/Plack-Handler/fcgi_cleanup_procmanager.t | 132 +++++++++++++++++++++ 2 files changed, 133 insertions(+) create mode 100644 t/Plack-Handler/fcgi_cleanup_procmanager.t diff --git a/cpanfile b/cpanfile index b5a1da66..190e541a 100644 --- a/cpanfile +++ b/cpanfile @@ -20,6 +20,7 @@ requires 'HTTP::Entity::Parser', 0.25; requires 'WWW::Form::UrlEncoded', 0.23; on test => sub { + requires 'Test::Exit'; requires 'Test::More', '0.88'; requires 'Test::Requires'; suggests 'Test::MockTime::HiRes', '0.06'; diff --git a/t/Plack-Handler/fcgi_cleanup_procmanager.t b/t/Plack-Handler/fcgi_cleanup_procmanager.t new file mode 100644 index 00000000..80f0704b --- /dev/null +++ b/t/Plack-Handler/fcgi_cleanup_procmanager.t @@ -0,0 +1,132 @@ +use strict; +use warnings; +use Test::More; + +plan skip_all => "release test only" unless $ENV{RELEASE_TESTING}; + +use Test::Exit; # before FCGI::ProcManager that calls exit +use Test::Requires qw(FCGI FCGI::ProcManager LWP::UserAgent); +use Plack; +use Plack::Util; +use Plack::Handler::FCGI; +use Test::TCP; +use lib 't/Plack-Handler'; +use FCGIUtils; + +my $ua_timeout = 3; + +test_lighty_external( sub { + my ($lighty_port, $fcgi_port, $needs_fix) = @_; + + test_tcp( + port => $fcgi_port, + server => sub { + my ($port) = @_; + my %c = run_server_cb($needs_fix)->($port); + + ok $c{enabled}, "Cleanup extension is enabled"; + + ok $c{handled}, "Cleanup handler ran successfully even though pm_post_dispatch handled TERM signal"; + ok !$c{handled_term}, "Didn't trigger our TERM cleanup_handler"; + ok !$c{response_cb}{ran}, "> had not run in response_cb"; + + is $c{response_cb}{handler_count}, 1, + "1 handler set up in response_cb, the TERM handler wasn't"; + + is $c{exit_code}, 0, + "proc_manager exited with 0"; + }, + client => sub { + # my ($port) = @_; Need to use the $lighty_port + + my $ua = LWP::UserAgent->new( timeout => $ua_timeout ); + my $res = $ua->get("http://127.0.0.1:$lighty_port/"); + my $response_received = time; + ok $res->is_success, "Got successful response"; + my $handled = $res->content; + is $handled, '0', "With response indicating not yet cleaned up"; + + # have to make the client wait until the server has exited + # otherwise the FCGI gets confused by sending a TERM + # that doesn't get handled right away. + sleep 1; + }, + ); +} ); + +done_testing(); + +sub run_server_cb { + my $needs_fix = shift; + + require Plack::Middleware::LighttpdScriptNameFix; + return sub { + my ($port) = @_; + + my %r = ( handled => 0 ); + + my $SIG_TERM; + local $SIG{TERM} = sub { $SIG_TERM->() if $SIG_TERM }; + + # An app inside an faux middleware + my $app = sub { + my ($env) = @_; + + $SIG_TERM = sub { + diag "app (pid $$) received signal TERM\n"; + $env->{'psgix.harakiri.commit'} = 1; + push @{ $env->{'psgix.cleanup.handlers'} }, sub {$r{handled_term} = 1}; + }; + + # The app + my $res = sub { + my ($env) = @_; + + $r{enabled} = $env->{'psgix.cleanup'}; + push @{ $env->{'psgix.cleanup.handlers'} }, + sub { $r{handled} = 1 }; + + kill TERM => $$; # trigger ProcManager TERM handler + + # Use streaming response to verify that cleanup happens + # even after that. + sub { shift->( [ 200, [], [ $r{handled} ] ] ) } + }->($env); + + Plack::Util::response_cb( $res, sub { + $r{response_cb} = { + enabled => $env->{'psgix.cleanup'}, + ran => $r{handled}, + handler_count => + scalar @{ $env->{'psgix.cleanup.handlers'} }, + }; + } ); + }; + + if ($needs_fix) { + note "Applying LighttpdScriptNameFix"; + $app = Plack::Middleware::LighttpdScriptNameFix->wrap($app); + } + + $| = 0; # Test::Builder autoflushes this. reset! + + # Initialize a ProcManager that's only a "server" + # without the "manager" part that forks. + my $manager = FCGI::ProcManager->new({ n_process => 0 }); + delete $manager->{PIDS}; + $manager->role("server"); + $manager->handling_init; + $manager->pm_notify("initialized"); + + my $fcgi = Plack::Handler::FCGI->new( + manager => $manager, + host => '127.0.0.1', + port => $port, + keep_stderr => 1, + ); + + $r{exit_code} = exit_code { $fcgi->run($app) }; + + return %r; + }; +} From 0b4648d13da7a2a1c90dce1dff0dba03e7ac293e Mon Sep 17 00:00:00 2001 From: Hu Yin Date: Tue, 14 Feb 2023 15:30:21 -0500 Subject: [PATCH 3/3] Update POD about cleanup hook --- lib/Plack/Handler/FCGI.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Plack/Handler/FCGI.pm b/lib/Plack/Handler/FCGI.pm index ae68ce9b..8a6d47d1 100644 --- a/lib/Plack/Handler/FCGI.pm +++ b/lib/Plack/Handler/FCGI.pm @@ -334,7 +334,7 @@ Supported L. Supports the C extension, in order to use it, just push a callback onto C<< $env->{'psgix.cleanup.handlers' >>. -These callbacks are run after the C hook. +These callbacks are run before the C hook. =item psgix.harakiri