puma
Add suport for rack.response_finished
Description
Looking to add support for racks response finished hooks.
rack.response_finished
An array of callables run by the server after the response has been processed. This would typically be invoked after sending the response to the client, but it could also be invoked if an error occurs while generating the response or sending the response; in that case, the error argument will be a subclass of Exception. The callables are invoked with +env, status, headers, error+ arguments and should not raise any exceptions. They should be invoked in reverse order of registration.
It is my understanding that the expected use of rack.after_reply and rack.response_finished are the same, so for this PR I opted to have them share the callable array instead of duplicating the after_reply behaviour just for response_finished.
Your checklist for this pull request
- [x] I have reviewed the guidelines for contributing to this repository.
- [x] I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
- [x] My pull request is 100 lines added/removed or less so that it can be easily reviewed.
- [ ] If this PR doesn't need tests (docs change), I added
[ci skip]to the title of the PR. - [ ] If this closes any issues, I have added "Closes
#issue" to the PR description or my commit messages. - [ ] I have updated the documentation accordingly.
- [ ] All new and existing tests passed, including Rubocop.
This is great. Being able to use the default "rack.response_finished" or Rack::RACK_RESPONSE_FINISHED as specified by rack would be super helpful, because we could make use of Rack::Lint and would help making people aware of the existence of this feature and would make people feel more confident about using it.
This looks like a good addition.
Looking at https://github.com/rack/rack/issues/1777, https://github.com/rack/rack/pull/1802, https://github.com/rack/rack/pull/1952, and https://github.com/Shopify/pitchfork/pull/98.
Not sure if the call loop should be updated to match what the pitchfork code did, I think one or more of the Puma call loops was/were changed to rescue within the loop rather than outside of it. That was part of the pitchfork PR.
Not sure if the call loop should be updated to match what the pitchfork code did
Adding more reliability to these hooks is a desirable trait in my opinion, should I make that change as part of this PR?
Just making note that, based on the SPEC, it appears that the callables should be executed in reverse order. I just opened a PR in pitchfork to fix this.
rack.response_finished should be added to Puma.
Add rack.after_reply to simplify logging and resources was added 20-Oct-2011. It also exists in Unicorn.
Since the rack spec does state 'They should be invoked in reverse order of registration', I think it would be best to reverse the call array for rack.response_finished, and leave rack.after_reply's behavior as is.
Given the current behavior, maybe Puma should run the rack.after_reply calls first, and then run the rack.response_finished calls in reverse order?
Lastly, as mentioned previously, should these calls be rescued inside or outside of the loop?