Skip to content

Support chunked encoding - #21

Merged
diazona merged 5 commits into
masterfrom
chunked-encoding/1/dev
Dec 31, 2021
Merged

Support chunked encoding#21
diazona merged 5 commits into
masterfrom
chunked-encoding/1/dev

Conversation

@diazona

@diazona diazona commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

I came up with this patch set a while ago for another project of mine that was experiencing errors only when receiving chunk-encoded data. It adds the ability to serve a chunked response. I know that users can do this themselves using a custom WSGI application, but since chunking is a part of the HTTP 1.1 standard, it's a common enough use case that it might be reasonable to handle it directly.

I'd be curious for your opinions @redtoad and @coordt about whether this makes sense to add to the project.

@diazona diazona added the enhancement New feature or request label Nov 19, 2021

@coordt coordt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like this change.

I would like to see some documentation for it in the readme, but the code looks fine.

@diazona

diazona commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

Ah yeah, good call. I'll add a mention of it.

At some point I think it'd be nice to split out the documentation into separate files and use Sphinx to generate proper package documentation which we can then upload to ReadTheDocs, leaving the README for the basics, like installation instructions and the simplest possible usage example. But that's a whole separate discussion.

@diazona

diazona commented Nov 21, 2021

Copy link
Copy Markdown
Contributor Author

I was actually just looking at the existing commented-out test case for gzip compression and I noticed it uses an attribute of httpserver itself to enable the compression, rather than basing it on the Accept-Encoding header. That got me thinking about whether it'd be better to do the same with chunking as well, i.e. make the user set httpserver.chunked to True or False to enable/disable chunking, instead of deciding whether to apply chunking based on the Transfer-Encoding header.

@redtoad if you have the chance, maybe you can comment on how intentional that decision was in the gzip case? (to use an attribute of httpserver instead of deciding to apply the encoding based on the header)

Aside from code clarity, the only functional advantage I can think of to using an attribute is that it allows clients to test whether they handle incorrectly encoded HTTP responses, like if a server applies chunking but doesn't set the Transfer-Encoding header, or vice versa. But I could imagine that some clients might want to do that, e.g. if someone is writing a library that has to deal with badly written HTTP servers. (The counterpoint is that they can always write a custom WSGI application for it... 🤷)

@redtoad

redtoad commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

@redtoad if you have the chance, maybe you can comment on how intentional that decision was in the gzip case? (to use an attribute of httpserver instead of deciding to apply the encoding based on the header)

If memory serves right I wanted to explicitly turn compression on/off and not base it on the request header.

Maybe that would something for an "enum": YES, NO, AUTO (i.e. if requested via header)

@diazona

diazona commented Nov 29, 2021

Copy link
Copy Markdown
Contributor Author

Good idea, I'll change it accordingly.

@diazona
diazona force-pushed the chunked-encoding/1/dev branch from e9a355a to 4abe099 Compare December 2, 2021 06:06
@diazona

diazona commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

I've implemented the version with the enum; thanks for that suggestion 😄

It breaks compatibility with Python 2.7 and 3.3, because the enum module was introduced in 3.4. We could add a dependency on the enum34 package (backport of enum) to continue supporting those older versions, but I'm feeling lazy and kind of hoping we can just drop support for 2.7 and 3.3 instead (see also #25).

@diazona
diazona force-pushed the chunked-encoding/1/dev branch from 4abe099 to 5867269 Compare December 11, 2021 13:04
@diazona diazona added this to the 0.6.0 milestone Dec 11, 2021
@diazona
diazona force-pushed the chunked-encoding/1/dev branch from 5867269 to d20ddae Compare December 16, 2021 19:25
@diazona

diazona commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

I've been trying this out in a private project and am getting a weird bug that is probably due to the other project's code, but I'm not sure. I'm hoping to get to the bottom of it before merging this, just in case there's an issue with the chunked encoding handling here.

@diazona diazona self-assigned this Dec 18, 2021
This allows passing an iterable of strings or bytes to serve_content()
so that it can be returned using chunked encoding.
This allows case-insensitive matching of header names.
This commit adds an implementation of chunk encoding to ContentServer.
The decision of whether to apply chunking is controlled by an enum,
which takes values YES (apply chunking), NO (don't apply chunking), or
AUTO (apply chunking only if requested by the Transfer-Encoding header).

I set the default value of the flag to NO for backwards compatibility,
but it could be changed to AUTO in the future, which would probably make
more sense.
This commit adds several test cases that confirm the server is able to
send data using the chunked transfer encoding, that it correctly does or
doesn't apply chunking depending on the value of the chunked attribute
and the Transfer-encoding header, that the Content-length header is
correctly omitted when sending a chunked response, and that various
combinations of content size and chunk size work correctly.
@diazona
diazona force-pushed the chunked-encoding/1/dev branch from d20ddae to 1ae3d6d Compare December 21, 2021 04:37
@diazona

diazona commented Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

I haven't fully gotten to the bottom of the issue I mentioned with my other project, but I think I've convinced myself that it lies in the other project's code, not here. So I'm good to merge this. @redtoad @coordt any further comments before I do?

@diazona
diazona merged commit 387eb4a into master Dec 31, 2021
@diazona
diazona deleted the chunked-encoding/1/dev branch December 31, 2021 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants