Run cleanup handers first, then post processing

[afresh1]

The FCGI::ProcManager pm_post_dispatch method may exit if it got a signal. Currently that will exit before handling the cleanup handlers. This is similar to harakiri, and the PSGI::Extensions documentation suggests that "it SHOULD [be implemented] in a way that cleanup handlers run before harakiri". Moving the cleanup handlers before the call to pm_post_dispatch should behave more correctly.

[miyagawa]

Hm, the comment suggests that when the proc manager exists that sends the TERM signal to workers and that is being handled in the cleanup handler. Isn't that the case you're talking about?

[afresh1]

Yes, when the manager exits, it sends its workers a TERM signal. That gets handled in the worker by the pm_post_dispatch method, which unfortunately currently happens before we process the cleanup handlers. The patch handles that signal after the cleanup handlers.

The cleanup handlers themselves have extra protection from a TERM signal as it's possible to either not have a manager at all. or to disable the signal handling in the manager. In either of those cases, the cleanup watches for that TERM signal and enables harakiri, so that it will exit whether there is a manager or not.

[miyagawa]

just curious, is there a reason not to move this after harakiri handling code either?

[afresh1]

At work we have some legacy hooks that happens in a ProcManager subclass' pm_post_dispatch before we call the parent method that exits. Moving this after the harakiri might cause that to get skipped and we'd have to refactor some things that aren't a priority at the moment.

[miyagawa]

Can you supply a unit test that shows the original problem?

[afresh1]

I expect we can figure that out.

[miyagawa]

Putting the discussion aside, you seem to be relying on some undocumented behaviors and ProcManager internals a lot. I'd fork the entire code into your own module and use it instead of using this, at least for now. I understand your desire to upstream the change and avoid a potential divergence, but this code has not been updated for years, so...

[afresh1]

We currently did fork and have an override with this change, but yes, would be much nicer to get it upstream and remove that.

[afresh1]

You probably got a notification, but we pushed up a test for this.