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 to disable Transfer-Encoding: chunked #1560

Merged
merged 5 commits into from
Apr 30, 2019
Merged

Allow to disable Transfer-Encoding: chunked #1560

merged 5 commits into from
Apr 30, 2019

Conversation

andreymal
Copy link
Contributor

The Transfer-Encoding: chunked is not allowed in some cases:

  • HTTP/1.0

  • a file streaming, because the Content-Length HTTP header is required here

This PR adds an option to disable it.

Closes #1194

(I hope that if self.chunked: expressions don't affect the performance)

@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #1560 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
+ Coverage    91.4%   91.44%   +0.03%     
==========================================
  Files          18       18              
  Lines        1804     1811       +7     
  Branches      344      348       +4     
==========================================
+ Hits         1649     1656       +7     
  Misses        131      131              
  Partials       24       24
Impacted Files Coverage Δ
sanic/response.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a4a3f6...7d6e60a. Read the comment docs.

status=200,
headers=None,
content_type="text/plain",
chunked=None,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense in the way you're using this variable to just make it a boolean instead of originally setting it to None?

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

And, also instead of this:

        chunked = self.chunked
        self.headers.pop("Content-Length", None)
        if chunked is None:
            chunked = version != "1.0"

        if chunked:

Why not something simpler without any variable assignment?

if self.chunked and version != "1.0"

Copy link
Contributor Author

@andreymal andreymal Apr 22, 2019

Choose a reason for hiding this comment

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

@ahopkins in general, I think there is a fundamental problem with HTTPResponse instances: they don't know an exact HTTP version (and some other info about the request) until the response is sent to the client. It forces me to use weird and complicated code like assignments inside the stream and get_headers methods instead of __init__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seemethere @ahopkins ok I tried to simplify it 7d6e60a

status=200,
headers=None,
content_type="text/plain",
chunked=None,
Copy link
Member

Choose a reason for hiding this comment

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

I agree.

And, also instead of this:

        chunked = self.chunked
        self.headers.pop("Content-Length", None)
        if chunked is None:
            chunked = version != "1.0"

        if chunked:

Why not something simpler without any variable assignment?

if self.chunked and version != "1.0"

@seemethere seemethere merged commit ef6d78c into sanic-org:master Apr 30, 2019
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.

Ability to send Content-Length header when streaming large files
3 participants