Opened 14 years ago

Closed 5 years ago

Last modified 5 years ago

#104 enhancement closed fixed (fixed)

mod_gzip type functionality for Twisted Web

Reported by: Nafai Owned by: therve
Priority: normal Milestone:
Component: web Keywords:
Cc: Nafai, spiv, jknight, hypatia, ibu Branch: branches/gzip-server-104-2
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description


Change History (23)

comment:1 Changed 14 years ago by 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

comment:2 Changed 13 years ago by hypatia

Assigning to jknight who is rewriting the http protocol.

comment:3 Changed 12 years ago by jknight

Implemented in web2.

comment:4 Changed 7 years ago by ibu

Cc: ibu added
Keywords: output compression gzip added
Resolution: fixed
Status: closedreopened

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

comment:5 Changed 7 years ago by <automation>

Owner: jknight deleted

comment:6 Changed 7 years ago by therve

Owner: set to therve
Status: reopenednew

comment:7 Changed 7 years ago by therve

Author: therve
Branch: branches/gzip-server-104

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

comment:8 Changed 7 years ago by therve

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

Refs #104

comment:9 Changed 6 years ago by therve

Keywords: output removed

comment:10 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added; compression gzip removed
Owner: therve deleted

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

comment:11 Changed 5 years ago by therve

Branch: branches/gzip-server-104branches/gzip-server-104-2

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

comment:12 Changed 5 years ago by David Reid

Owner: set to David Reid
Status: newassigned

comment:13 Changed 5 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid to therve
Status: assignednew

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.)

comment:14 Changed 5 years ago by therve

Keywords: review added
Owner: therve deleted
  1. I added a section to the howto.
  1. Sorry, but I fail to see the difference. Can you explain what you prefer a bit more?
  1. Done.
  1. Fixed.
  1. 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.
  1. 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.

comment:15 Changed 5 years ago by 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?

comment:16 Changed 5 years ago by therve

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

comment:17 Changed 5 years ago by Glyph

Owner: set to Glyph

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

comment:18 Changed 5 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to therve

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.

comment:19 Changed 5 years ago by therve

Resolution: fixed
Status: newclosed

(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.

comment:20 Changed 5 years ago by 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.

comment:21 in reply to:  20 ; Changed 5 years ago by 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.)

comment:22 in reply to:  21 ; Changed 5 years ago by 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?

comment:23 in reply to:  22 Changed 5 years ago by 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.

Note: See TracTickets for help on using tickets.