New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure response headers are being sent before closing the response #75

Merged
merged 6 commits into from Apr 4, 2018

Conversation

Projects
None yet
2 participants
@josejulio
Contributor

josejulio commented Feb 8, 2018

Fixes cherrypy/cherrypy#1404

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bugfix

  • What is the related issue number (starting with #)

cherrypy/cherrypy#1404

  • What is the current behavior? (You can also link to an open issue here)

on_end_request hook is being call before sending a response on cherrypy

  • What is the new behavior (if this is a feature change)?

on_end_request hook is being call after sending a response on cherrypy

  • Other information:

I found Lawouach/WebSocket-for-Python#158 which lead me to this "PR" https://bitbucket.org/cherrypy/cherrypy/pull-requests/121/fixing-issue-1404-on_end_request-hook-is/diff
I ported that to cheroot and addressed the last comment from "Sylvain Hellegouarch"

@josejulio josejulio force-pushed the josejulio:on_end_request_before_response branch 2 times, most recently from 959fd81 to ebedc24 Feb 8, 2018

@codecov

This comment has been minimized.

codecov bot commented Feb 9, 2018

Codecov Report

Merging #75 into master will increase coverage by 0.33%.
The diff coverage is 96.96%.

@@           Coverage Diff            @@
##           master    #75      +/-   ##
========================================
+ Coverage   65.67%    66%   +0.33%     
========================================
  Files          15     15              
  Lines        2753   2780      +27     
========================================
+ Hits         1808   1835      +27     
  Misses        945    945

@webknjaz webknjaz requested review from jaraco, webknjaz, Lawouach and cherrypy/contributors Feb 9, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Feb 9, 2018

Original PR authors/reviewers seem to be: @moigagoo @ronin8600 @Lawouach

@webknjaz

This comment has been minimized.

Member

webknjaz commented Feb 9, 2018

Thanks for the PR.
I cannot accept it w/o manually checking the flow you're changing, so I'll do this once I have some time. This should also trigger checks against CherryPy itself.
Covering this with tests might speed up the process.

@josejulio

This comment has been minimized.

Contributor

josejulio commented Feb 9, 2018

Thanks @webknjaz
I'll try to make time to write test.
Basically this writes the headers before closing the request.
See here and here

I suppose (haven't digged much on cherrypy code) cherrypy hooks on that close and triggers the on_end_request

@josejulio josejulio force-pushed the josejulio:on_end_request_before_response branch 2 times, most recently from 482f05b to 562913c Feb 13, 2018

@josejulio josejulio changed the title from Ensure response headers are being sent before calling on_end_request hook to Ensure response headers are being sent before closing the response Feb 13, 2018

@josejulio

This comment has been minimized.

Contributor

josejulio commented Feb 13, 2018

@webknjaz I added a test, I verified that it fails without this patch - and it passes with the patch :) -

Let me know how this can be improved.

@josejulio josejulio force-pushed the josejulio:on_end_request_before_response branch from 562913c to c17bff6 Feb 13, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Feb 13, 2018

@josejulio first of all, linters in CI fail: https://travis-ci.org/cherrypy/cheroot/jobs/340814348#L484
We use fail fast approach in our CI to save on computing resources and power, so tests are not even being run if fast checks crash.
Could you please correct this?

P.S. Also, it's possible to pip install pre-commit && pre-commit install locally to set up automatic linter running for each commit you do on your machine :) So that you won't wait for CI to be triggered and return the result.

@webknjaz

Plus code needs to bypass linters

def test_send_header_before_closing(testing_server_close):
"""Tests we are actually sending the headers before closing the response."""
testing_server_close.server_client.get('/')

This comment has been minimized.

@webknjaz

webknjaz Feb 13, 2018

Member

How about checking http response headers in client?

resp_status_line, resp_headers, resp_body = testing_server_close.server_client.get('/')

This comment has been minimized.

@josejulio

josejulio Feb 13, 2018

Contributor

Sorry, maybe my description is wrong here or i didn't understand well.

The headers are always sent, the problem is that the close [1] method is being call before the headers are being sent.

This is a problem in cherrypy, because the on_end_request is being called before the headers are actually sent, anything sent to the client from this hook will reach before the headers, thus the headers will be malformed. I don't think I can test this from the client side unless i can get a raw response, but i would still need to send something on that close [1] method.

It's done in a similar fashion in pep-0333 [2] (talking about the fix, not the tests)

    try:
        for data in result:
            if data:    # don't send headers until body appears
                write(data)
        if not headers_sent:
            write('')   # send headers now if body was empty
    finally:
        if hasattr(result, 'close'):
            result.close()

[1] https://github.com/cherrypy/cheroot/pull/75/files#diff-08d927a6f63bcf494ed88cca2a6873d0R144
[2] https://www.python.org/dev/peps/pep-0333/

def close(self):
"""Hook for close, tests if headers already sent."""
self.sent_headers_before_closing = \
self.req is not None and self.req.sent_headers

This comment has been minimized.

@webknjaz

webknjaz Feb 13, 2018

Member

This seems to be overengineered. Please see my comment below.

@josejulio josejulio force-pushed the josejulio:on_end_request_before_response branch from 376de01 to bb2ddde Feb 16, 2018

@josejulio

This comment has been minimized.

Contributor

josejulio commented Feb 16, 2018

@webknjaz I fixed linting issues and tried to simplify the test.

Here is the output of the test without the fix (commit: 742b1b9)

―――――――――――――――――――――――――――――――――――――― test_send_header_before_closing ――――――――――――――――――――――――――――――――――――――

testing_server_close = <cheroot.wsgi.Server object at 0x7fc57f98a350>

    def test_send_header_before_closing(testing_server_close):
        """Test we are actually sending the headers before calling 'close'."""
        _, _, resp_body = testing_server_close.server_client.get('/')
>       assert resp_body == 'hello'
E       AssertionError: assert 'helloHTTP/1....\r\n0\r\n\r\n' == 'hello'
E         + hello
E         - helloHTTP/1.1 200 OK\r
E         - Content-Type: text/html\r
E         - Transfer-Encoding: chunked\r
E         - Date: Fri, 16 Feb 2018 05:30:56 GMT\r
E         - Server: Cheroot/6.0.1.dev103+gc17bff6d\r
E         - \r...
E         
E         ...Full output truncated (3 lines hidden), use '-vv' to show

cheroot/test/test_core.py:402: AssertionError

@josejulio

This comment has been minimized.

Contributor

josejulio commented Mar 1, 2018

@webknjaz @jaraco @Lawouach
Is there anything else I can do here? Do you have additional comments on the tests?
Thanks.

@@ -973,9 +973,6 @@ def respond(self):
self.server.gateway(self).respond()
if (self.ready and not self.sent_headers):
self.sent_headers = True
self.send_headers()

This comment has been minimized.

@webknjaz

webknjaz Mar 21, 2018

Member

Why completely remove this? How will it affect non-WSGI plain HTTP server?

This comment has been minimized.

@josejulio

josejulio Mar 22, 2018

Contributor

I though that adding it to self.server.gateway(self).respond() would cover it

I suppose I can add it back to ensure we are always sending the headers in case the gateway is not sending the headers.

def close(self):
"""Hook for close, write hello."""
self.req.write('hello'.encode('utf-8'))

This comment has been minimized.

@webknjaz

webknjaz Mar 21, 2018

Member

b'hello' would look nicer

@@ -137,6 +137,9 @@ def respond(self):
raise ValueError('WSGI Applications must yield bytes')
self.write(chunk)
finally:
# Send headers if not already sent
if not self.req.sent_headers:

This comment has been minimized.

@webknjaz

webknjaz Mar 21, 2018

Member

It's always paired with "if not sent" check. Try moving it inside the method and maybe rename it into smth like ensure_headers_sent or so

@webknjaz

This comment has been minimized.

Member

webknjaz commented Mar 21, 2018

@josejulio sorry for late reply, I've added some thoughts on this

@josejulio

This comment has been minimized.

Contributor

josejulio commented Mar 21, 2018

@josejulio sorry for late reply, I've added some thoughts on this

Better late than never :).
I'll address your comments later today, thanks!

Addressed review comments
  Moved the test if the headers are sent inside the ensure_headers_sent
   (renamed from write_headers)
  Puts back a "send headers" test.
@josejulio

This comment has been minimized.

Contributor

josejulio commented Mar 22, 2018

@webknjaz Addressed the comments, travis is green and this is ready for other review round, thanks!

@@ -972,10 +972,8 @@ def respond(self):
self.rfile = KnownLengthRFile(self.conn.rfile, cl)
self.server.gateway(self).respond()
self.ensure_headers_sent()

This comment has been minimized.

@webknjaz

webknjaz Mar 26, 2018

Member

It looks like you've lost ready check here. Could be:

self.ready and self.ensure_headers_sent()

Is this intentional? What is the effect of a such change?

This comment has been minimized.

@josejulio

josejulio Mar 29, 2018

Contributor

Not intentional, missed that, thanks for spotting it!

@josejulio

This comment has been minimized.

Contributor

josejulio commented Apr 4, 2018

@webknjaz Forgot to say that addressed your latest comments, thanks

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 4, 2018

LGTM! Thank you @josejulio!

@webknjaz webknjaz merged commit 513f789 into cherrypy:master Apr 4, 2018

8 checks passed

WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
ci/circleci: macos-build Your tests passed on CircleCI!
Details
codeclimate 1 fixed issue
Details
codecov/patch 96.96% of diff hit (target 65.67%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 4, 2018

@josejulio This code should reach PYPI soon, when this build is complete: https://travis-ci.org/cherrypy/cheroot/builds/362369631

It should become available at https://pypi.org/project/Cheroot/6.1.0/ in about 30 min from now.

@josejulio

This comment has been minimized.

Contributor

josejulio commented Apr 5, 2018

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment