Skip to content
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

Allow 204 responses without Content-Length/Transfer-Encoding #1743

Merged
merged 3 commits into from Jul 1, 2016

Conversation

nickcoutsos
Copy link
Contributor

These changes relate to #1736:

  • http1connection now also considers 204 to decide when chunking isn't necessary
  • RequestHandler now also asserts the write buffer is empty for 204 responses

@@ -322,12 +330,11 @@ def test_options_request(self):
def test_no_content(self):
response = self.fetch("/no_content")
self.assertEqual(response.code, 204)
# 204 status doesn't need a content-length, but tornado will
# add a zero content-length anyway.
# 204 status shouldn't have a content-length
#
# A test without a content-length header is included below
# in HTTP204NoContentTestCase.
Copy link
Member

Choose a reason for hiding this comment

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

There used to be two tests related to 204: This one in which the Content-Length header was used, and HTTP204NoContentTestCase, which manipulated the connection manually to create responses without Content-Length. Now HTTP204NoContentTestCase is redundant; it should be reversed and become the test of responses with the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was a little unsure about how best to organize it. Wasn't this test already covering that by doing its followup request for "/no_content?error=1"? Or do you mean to move that part of the test into the other test case (and rename I suppose)?

Copy link
Member

Choose a reason for hiding this comment

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

There are three cases: no header (fine), Content-Length: 0 (fine), and non-zero content-length (error). We no longer have a case demonstrating the acceptance of Content-Length: 0, and the error case is made awkward by the need to write directly to the connection from a RequestHandler (which I think will break things in http2). I'm suggesting that this test just cover the simple case (no header), and both cases in which the header is present move to HTTP204NoContentTestCase.

@bdarnell bdarnell merged commit 57d15bb into tornadoweb:master Jul 1, 2016
@bdarnell
Copy link
Member

bdarnell commented Jul 1, 2016

Thanks!

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.

None yet

2 participants