Implement support for the PSGI environment cleanup handlers proposal #52

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@avar

avar commented Sep 11, 2012

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.

Consider this an RFC, but maybe it's good to merge as-is.

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.
@avar

This comment has been minimized.

Show comment Hide comment
@avar

avar Sep 12, 2012

Will send a new pull request.

avar commented Sep 12, 2012

Will send a new pull request.

@avar avar closed this Sep 12, 2012

@miyagawa

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Dec 27, 2012

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.

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.

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Dec 27, 2012

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

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

This comment has been minimized.

Show comment Hide comment
@rmesser

rmesser Feb 12, 2013

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.

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.

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Feb 12, 2013

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.

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.

This comment has been minimized.

Show comment Hide comment
@rmesser

rmesser Feb 12, 2013

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?

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?

This comment has been minimized.

Show comment Hide comment
@avar

avar Feb 12, 2013

Owner
Owner

avar replied Feb 12, 2013

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Feb 12, 2013

@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.

@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.

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Feb 12, 2013

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.

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.

This comment has been minimized.

Show comment Hide comment
@rmesser

rmesser Feb 12, 2013

This comment has been minimized.

Show comment Hide comment
@rmesser

rmesser Feb 12, 2013

@miyagawa

This comment has been minimized.

Show comment Hide comment
@miyagawa

miyagawa Dec 27, 2012

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

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