mod_gzip type functionality for Twisted Web

[Nafai]

As I understand it, mod_gzip uses Content Encoding of gzip
if, after querying the browser, it finds that it is
acceptable.  I used this on a mostly text-only dynamic site
and it improved the speed of it immensely.  Great way to
improve the apparent speed of Twisted Web

[hypatia]

Assigning to jknight who is rewriting the http protocol.

[jknight]

Implemented in web2.

[ibu]

As far as I see, gzip compression is not yet implemented in twisted.web and thus missing.

[therve]

(In [31686]) Branching to 'gzip-server-104'

[therve]

(In [31687]) Import gzip support in server.Request

Refs #104

[itamar]

So... I guess I can put this up for review? Remove keyword if it shouldn't be.

[therve]

(In [34686]) Branching to 'gzip-server-104-2'

[dreid]

Great job.

• Lacks prose documentation.
• Test docstrings are inconsistent. We should prefer the form used by GzipEncoderTests.test_nonEncoding.
• The docstring for Site.encoders lacks links to the to the interface expected to be provided by objects in it.
• This branch adds a lot of public API unspecified by interfaces.
• GzipEncoderFactory should implement a private Interface. To quote glyph "You can have one, but you can't make one."
• GzipEncoder should also implement a private Interface and should possibly be private itself?
• This should be configurable from the command line. (Or at least have another ticket filed to add support on the commandline.)

[therve]

1. I added a section to the howto.

2. Sorry, but I fail to see the difference. Can you explain what you prefer a bit more?

3. Done.

4. Fixed.

5. 6. I added public interfaces and made GzipEncoder private. Those interfaces are meant to be implemented by third party if necessary, so I didn't really understand why they should be private.

7. I'll open a new ticket afterwards. I guess you meant for "twistd web" ? AFAICT we could just put the encoder by default in there, it shouldn't break anything.

[glyph]

Apropos of all the hullabaloo about chosen-plaintext attacks: should we be worried about enabling this for dynamic (i.e. attacker-controlled) content? Enable it only for static resources or something?

[therve]

I think we can document it, but let the user do whatever they want with it.

[glyph]

I just  forced the build (from orbit, it's the only way to be sure).

[glyph]

Therve,

Once again, thanks!!

re: points 5 and 6 above, we wanted the interface to be private so that third parties could not provide their own encoders; we want to provide this specific bit of functionality first, but not expose a general system based on the hackish twisted.web resource model; in the future something like a compressing tube or just using IProtocol would be a much better, easier to support, protocol-agnostic interface. But nobody really needs third-party content encodings anyway; the mechanism is really just used for compression and we're implementing it directly.

That said, if you feel strongly about wanting to provide other content encodings, go ahead. However:

1. It is literally against the twisted coding standard to have a method called handle (also: foo(), process (with the exemption for the existing method), doIt, etc. If this is not written down anywhere you can find it, trust me, it is. Consider: encode, instead.
2. Similarly: handleRequest should follow existing convention and be buildEncoder or encoderForRequest.
3. The documentation should mention chosen-plaintext attacks.

And, optionally:

1. twisted.web.test.test_web.StaticResource seems like maybe it's a little silly. You could just use a static.Data and it would be less code. (Lately I am trying to raise the issue of redundant test fixtures, stubs, mocks, etc, as much as possible.)

I don't think this needs another trip through review; coverage looks adequate, tests pass, and I trust your discretion.

[therve]

(In [35786]) Merge gzip-server-104-2

Author: therve Reviewers: dreid, glyph Fixes: #104

Site.encoders now can contain a list of supported encoders for the Site instance, starting with gzip.

[therve]

Thanks a lot for the review! I've handled all the points except the bit about chose-plaintext attacks, because I don't understand how it's relevant. Please open a ticket and assign it to me if there is an update to make.

[glyph]

Replying to therve:

Thanks a lot for the review! I've handled all the points except the bit about chose-plaintext attacks, because I don't understand how it's relevant. Please open a ticket and assign it to me if there is an update to make.

It looks like the documentation that you merged links to the public names for the now-private interfaces; that should be fixed. (Also possibly a comment about the supported-ness of those interfaces.)

[therve]

Replying to glyph:

It looks like the documentation that you merged links to the public names for the now-private interfaces; that should be fixed. (Also possibly a comment about the supported-ness of those interfaces.)

Sorry, can you point to one of those issues and what's the possible fix?

[glyph]

Replying to therve:

Replying to glyph:

It looks like the documentation that you merged links to the public names for the now-private interfaces; that should be fixed. (Also possibly a comment about the supported-ness of those interfaces.)

Sorry, can you point to one of those issues and what's the possible fix?

If you have a look at http://twistedmatrix.com/trac/changeset/35786#file0 you will notice that it says IRequestEncoderFactory / IRequestEncoder where it should say _IRequestEncoderFactory / IRequestEncoder. The fix is just to add the underscore. Additional text saying that you can implement your own, but the interface may change out from under you, would also be good.