Skip to content

Add support for the psgix.cleanup extension#610

Merged
miyagawa merged 2 commits intoplack:masterfrom
GrantStreetGroup:Add_psgix.cleanup_support
Feb 10, 2018
Merged

Add support for the psgix.cleanup extension#610
miyagawa merged 2 commits intoplack:masterfrom
GrantStreetGroup:Add_psgix.cleanup_support

Conversation

@afresh1
Copy link
Contributor

@afresh1 afresh1 commented Feb 9, 2018

Having cleanup support in the FCGI Handler will be beneficial to us as it allows post-processing of tasks after the request has been served to the client and we would like to do our request logging at that point.

Allows post-processing of tasks after the request has been served to
the client.
$proc_manager && $proc_manager->pm_post_dispatch();

{
local $SIG{TERM} = sub {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure what you're trying to achieve with this. Maybe you're trying to capture stuck cleanup handlers? In that case, pushing another callback won't actually help, because the for loop only runs once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the case where the fcgi-manager exits and sends a TERM to the workers while the worker is busy processing the cleanup handlers. My testing shows in that case the worker wouldn't actually exit. I didn't attempt to track down why, just that this fixed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, pushing to psgix.cleanup.handlers array has no effect because when a cleanup handler is running, the code is already inside the for loop for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, apparently I should not be relying on an implementation detail. Oops. My use-case has the manager, so this was a bit of an oversight. (although it does work in my testing)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, yeah, i'm surprised to see that it actually works, but yeah, i think it should be better not relying on it and use a regular variable and flags for this. I would also avoid using $_ for no special reasons and use a regular for my $handler loop.

Also can you add a source comment explaining this TERM logic that you described above? Thanks,

Document why we trap the TERM signal, and stop using an undocumented,
unsupported perl version to push the exit inside of the cleanup
handlers.

While here, avoid using $_ and instead use a lexical loop variable.
@miyagawa miyagawa merged commit 62b3e31 into plack:master Feb 10, 2018
@miyagawa
Copy link
Member

Thanks!

@afresh1 afresh1 deleted the Add_psgix.cleanup_support branch February 12, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants