Opened 11 years ago

Closed 2 years ago

Last modified 2 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
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description


Change History (23)

comment:1 Changed 11 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 10 years ago by hypatia

Assigning to jknight who is rewriting the http protocol.

comment:3 Changed 9 years ago by jknight

Implemented in web2.

comment:4 Changed 4 years ago by ibu

  • Cc ibu@… added
  • Keywords output compression gzip added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:5 Changed 4 years ago by <automation>

  • Owner jknight deleted

comment:6 Changed 4 years ago by therve

  • Owner set to therve
  • Status changed from reopened to new

comment:7 Changed 4 years ago by therve

  • Author set to therve
  • Branch set to branches/gzip-server-104

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

comment:8 Changed 4 years ago by therve

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

Refs #104

comment:9 Changed 3 years ago by therve

  • Keywords output removed

comment:10 Changed 2 years ago by itamar

  • 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 2 years ago by therve

  • Branch changed from branches/gzip-server-104 to branches/gzip-server-104-2

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

comment:12 Changed 2 years ago by dreid

  • Owner set to dreid
  • Status changed from new to assigned

comment:13 Changed 2 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to therve
  • Status changed from assigned to new

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 2 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 2 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 2 years ago by therve

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

comment:17 Changed 2 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 2 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 2 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(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 follow-up: Changed 2 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 ; follow-up: Changed 2 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 ; follow-up: Changed 2 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 2 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.