Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Implement support for the PSGI environment cleanup handlers proposal #53

Open
wants to merge 1 commit into from

3 participants

@avar

This should address the issues noted in #52

Implement the proposal at
https://github.com/plack/psgi-specs/wiki/Proposal:-PSGI-environment-cleanup-handlers. This
seems to work for me, at least the subroutines get run, and you can
e.g. "sleep 10" in the cleanup handler without holding up the request.

One potential caveat here is that by passing the $env hash to the
post_client_connection_hook routine we might be causing something
evil, but I haven't spotted any problems with it so far.

Note that we have to re-check the harakiri flag in
post_client_connection_hook because the cleanup handlers might set it.

@avar avar Implement support for the PSGI environment cleanup handlers proposal
Implement the proposal at
https://github.com/plack/psgi-specs/wiki/Proposal:-PSGI-environment-cleanup-handlers. This
seems to work for me, at least the subroutines get run, and you can
e.g. "sleep 10" in the cleanup handler without holding up the request.

One potential caveat here is that by passing the $env hash to the
post_client_connection_hook routine we might be causing something
evil, but I haven't spotted any problems with it so far.

Note that we have to re-check the harakiri flag in
post_client_connection_hook because the cleanup handlers might set it.
a7a2b2f
@miyagawa

You may not be the right person to be asked/bugged about this, but it seems that if keep-alive is in the place, the post connection client hook is executed only when the current session is ended. While that makes sense to not interrupt the client, but your code only has one cleanup handlers buffer per client, which I guess probably won't work if multiple requests are processed (the last one overwrites the cleanup handlers stack).

I think the right/cleaner way to do this is to disable keep-alive if harakiri or cleanup is set, and then in the post client disconnect hook, check for the cleanup handlers to run, then check harakiri, instead of doing the check multiple times.

Owner

Another way to make things simpler is to disable cleanup (set psgix.cleanup to false) when keep-alive is enabled, so that if a developer wants to use the cleanup handlers they have to turn off keepalive explicitly with --disable-keepalive

I am using a modified version of this change for a project I am working on -- actually we created a custom subclass of Starman::Server to enable cleanup handlers, but the concept is the same. And you are correct @miyagawa, the post_client_connection_hook does not run if keep-alive is set and another request comes in before the keep-alive is dropped. So that's bad, and led to me wasting some time debugging, but also led me back to this code and your comments, so not all bad if I can at least save somebody else the trouble.

Anyway, your proposed solution of entirely disabling keep-alive if you want to use cleanup handlers isn't ideal for us, since we'd rather not remove keep alive and have reduced performance just so that we can occasionally add cleanup handlers. So instead in our code we make sure to always set $env->{'psgix.harakiri.commit'} = 1 whenever we add a cleanup handler. That way the cleanup handler always runs right after that request is processed. We lose keep-alive only for that one request, which is what we want anyway since keep-alive is not helpful if we're about to start a long-running process.

So I'd suggest that somewhere in processing before the post_client_connection_hook, Starman::Server should check to see whether there is at least one entry in psgix.cleanup.handlers. If so, set psgix.harakiri.commit = 1, and then I think you can avoid making general rules that would force always disabling keep-alive in order to use clean-up handlers.

Owner

So instead in our code we make sure to always set $env->{'psgix.harakiri.commit'} = 1 whenever we add a cleanup handler.

Adding harakiri.commit will stop the entire process for the worker, a very different thing from disabling keep-alive and resetting the connection.

Ok, so maybe my suggestion isn't a good general solution. Still, it works for us -- we're ok with ending the life of that worker early because we don't add clean-up handlers that often. So even if a few workers die an early death, our overall performance will still be better than if we turned keep-alive off entirely.

But maybe there is some other better way to tell Starman to drop that one connection but still keep that worker process going?

Owner

@rmesser are you using keep-alive between the frontend and backend, like nginx-starman? In case of nginx it's relatively new code that is disabled by default, and other frontends such as Apache + mod_proxy has a serious issue with the persistent connections.

In the production setup I usually do not recommend keep-alives.

Owner

Still, it works for us -- we're ok with ending the life of that worker early because we don't add clean-up handlers that often

psgix.cleanup is meant to do a post-request connection hook, and changing that to commit harakiri is to change the semantics of the flag and I do not agree to that kind of a change.

You're right that disabling keep-alive at all and disabling keep-alive only when cleanup handler is set are two different things: hence my original (first comment) suggestion:

I think the right/cleaner way to do this is to disable keep-alive if harakiri or cleanup is set,

But I also recommend against keep-alives in production in general (see my comment above), hence the second suggestion. Either way works for me, and we could support both.

@miyagawa

See above - i think you can only pass $env but not the handlers itself, so that disconnect hook can check on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 12, 2012
  1. @avar

    Implement support for the PSGI environment cleanup handlers proposal

    avar authored
    Implement the proposal at
    https://github.com/plack/psgi-specs/wiki/Proposal:-PSGI-environment-cleanup-handlers. This
    seems to work for me, at least the subroutines get run, and you can
    e.g. "sleep 10" in the cleanup handler without holding up the request.
    
    One potential caveat here is that by passing the $env hash to the
    post_client_connection_hook routine we might be causing something
    evil, but I haven't spotted any problems with it so far.
    
    Note that we have to re-check the harakiri flag in
    post_client_connection_hook because the cleanup handlers might set it.
This page is out of date. Refresh to see the latest.
Showing with 111 additions and 0 deletions.
  1. +26 −0 lib/Starman/Server.pm
  2. +19 −0 t/harakiri.t
  3. +66 −0 t/psgix_cleanup.t
View
26 lib/Starman/Server.pm
@@ -191,6 +191,8 @@ sub process_request {
'psgix.io' => $conn,
'psgix.input.buffered' => Plack::Util::TRUE,
'psgix.harakiri' => Plack::Util::TRUE,
+ 'psgix.cleanup' => Plack::Util::TRUE,
+ 'psgix.cleanup.handlers' => [],
};
# Parse headers
@@ -431,11 +433,20 @@ sub _prepare_env {
sub _finalize_response {
my($self, $env, $res) = @_;
+ # We also check for harakiri in post_client_connection_hook()
+ # after the cleanup handlers have run.
if ($env->{'psgix.harakiri.commit'}) {
$self->{client}->{keepalive} = 0;
$self->{client}->{harakiri} = 1;
}
+ if (@{$env->{'psgix.cleanup.handlers'}}) {
+ $self->{client}->{env_and_cleanup_handlers} = {
+ env => $env,
+ cleanup_handlers => $env->{'psgix.cleanup.handlers'}
+ };
+ }
+
my $protocol = $env->{SERVER_PROTOCOL};
my $status = $res->[0];
my $message = status_message($status);
@@ -525,6 +536,21 @@ sub _finalize_response {
sub post_client_connection_hook {
my $self = shift;
+
+ if ($self->{client}->{env_and_cleanup_handlers}) {
+ my ($env, $cleanup_handlers) = @{$self->{client}->{env_and_cleanup_handlers}}{qw(env cleanup_handlers)};
+
+ for my $cleanup_handler (@$cleanup_handlers) {
+ $cleanup_handler->($env);
+ }
+
+ # We also check for harakiri in _finalize_response()
+ # after the request has been completed.
+ if ($env->{'psgix.harakiri.commit'}) {
+ $self->{client}->{harakiri} = 1;
+ }
+ }
+
if ($self->{client}->{harakiri}) {
exit;
}
View
19 t/harakiri.t
@@ -39,4 +39,23 @@ test_psgi
is keys(%seen_pid), 23, 'In Harakiri mode, each pid only used once';
};
+test_psgi
+ app => sub {
+ my $env = shift;
+ push @{$env->{'psgix.cleanup.handlers'}} => sub {
+ my ($cleanup_env) = @_;
+ $cleanup_env->{'psgix.harakiri.commit'} = 1;
+ };
+ return [ 200, [ 'Content-Type' => 'text/plain' ], [$$] ];
+ },
+ client => sub {
+ my %seen_pid;
+ my $cb = shift;
+ for (1..23) {
+ my $res = $cb->(GET "/");
+ $seen_pid{$res->content}++;
+ }
+ is keys(%seen_pid), 23, 'In Harakiri mode triggered by cleanup, each pid only used once';
+ };
+
done_testing;
View
66 t/psgix_cleanup.t
@@ -0,0 +1,66 @@
+use strict;
+use warnings;
+
+use HTTP::Request::Common;
+use Plack::Test;
+use Test::More;
+
+$Plack::Test::Impl = 'Server';
+$ENV{PLACK_SERVER} = 'Starman';
+
+test_psgi
+ app => sub {
+ my $env = shift;
+ return [ 200, [ 'Content-Type' => 'text/plain' ], [$env->{'psgix.cleanup'}] ];
+ },
+ client => sub {
+ my $cb = shift;
+ my $res = $cb->(GET "/");
+ ok($res->content, "We set psgix.cleanup");
+ };
+
+test_psgi
+ app => sub {
+ my $env = shift;
+ return [ 200, [ 'Content-Type' => 'text/plain' ], [ref($env->{'psgix.cleanup.handlers'})] ];
+ },
+ client => sub {
+ my $cb = shift;
+ my $res = $cb->(GET "/");
+ cmp_ok($res->content, "eq", "ARRAY", "psgix.cleanup.handlers is an array");
+ };
+
+test_psgi
+ app => sub {
+ my $env = shift;
+ return [ 200, [ 'Content-Type' => 'text/plain' ], [join "", @{$env->{'psgix.cleanup.handlers'}}] ];
+ },
+ client => sub {
+ my $cb = shift;
+ my $res = $cb->(GET "/");
+ cmp_ok($res->content, "eq", "", "..which is empty by default");
+ };
+
+my $content = "NO_CLEANUP";
+test_psgi
+ app => sub {
+ my $env = shift;
+ push @{$env->{'psgix.cleanup.handlers'}} => sub { $content .= "XXX" };
+ push @{$env->{'psgix.cleanup.handlers'}} => sub { $content .= "YYY" };
+ push @{$env->{'psgix.cleanup.handlers'}} => sub { $content .= "ZZZ" };
+ return [ 200, [ 'Content-Type' => 'text/plain' ], [ $content ] ];
+ },
+ client => sub {
+ my $cb = shift;
+ my $res = $cb->(GET "/");
+ cmp_ok($res->content, "eq", "NO_CLEANUP", "By the time we run the cleanup handler we've already returned a response");
+
+ my $responses;
+ for (1..10) {
+ my $res = $cb->(GET "/");
+ $responses .= $res->content;
+ }
+ like($responses, qr/$_/, "The response contains '$_' indicating the cleanup handlers were run") for qw(XXX YYY ZZZ);
+ };
+
+done_testing;
Something went wrong with that request. Please try again.