[Zope-dev] ZPublisher events before streaming a response

[Martin Aspeli]

Tres Seaver wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Aspeli wrote:
>> Hi,
>>
>> In some cases (e.g. large OFS.File/Image responses), Zope 2 will use
>> response.write() to stream the response.
>>
>> We have events that fire before and after a "regular" response is
>> returned, but none that allow us to set headers (caching headers, in
>> this case) before such a streaming response is calculated. The normal
>> events fire too late.
>>
>> We'd therefore like to add a new event in the HTTPResponse class (in
>> ZServer, though I think it makes sense to add to the ZPublisher base
>> class version as well). It'd hook in something like this:
>>
>>       if not self._wrote:
>>
>>           # new event code
>>           site = getSite()
>>           request = getattr(site, 'REQUEST', None)
>>           notify(PubBeforeStreaming(request))
>>
>>           # continue as before...
>>
>> (I couldn't find a better way to get hold of the request from a method
>> in the response, without adding a dependency on five.globalrequest,
>> which I assume is not desirable).
>>
>> Any objections? We need this in Zope 2.12, though I'll obviously merge
>> to trunk, too.
>
> I don't understand the need.  The OFS.Image module uses RESPONSE.write
> in three methods:
>
> - - _range_request_handler
>
> - - index_html
>
> - - manage_FTPget
>
> Of these, only 'index_html' is reasonable for setting HTTP cache
> headers, and it already has built-in support for that, via the
> ZCacheable API.  In particular, this support is how the "file stream
> iterator" bits get done, which is way more optimal than falling through
> to the RESPONSE.write calls.

plone.caching provides a centralised, sane way to set caching headers 
(and deal with other caching related features) across a number of 
different scenarios. Right now, it works for everything except "large" 
OFS files that trigger streamed responses.

You're right that the ZCacheable API can be used to manage headers here, 
by associating objects with an HTTP cache manager. However, if you look 
at the number of hoops CacheFu/Products.CacheSetup (which is what 
plone.caching aims to replace) had to jump through to use that API to 
manage headers and still centralise policy, it's a right mess.

It's also pretty obtuse how it works. In the index_html method in 
OFS.Image.File, you'll see calls to ZCacheable_get() and 
ZCacheable_set(). Somewhere inside those functions, headers are 
manipulated. That's not exactly obvious. Dig deeper into the standard 
cache managers, and the code continues to be weird.

I think the ZCacheable API makes sense (although it's a bit clunky) for 
implementing *caches*. That is, you calculate a cache key and look for a 
cached copy of something. Its use for HTTP header manipulation seems 
pretty forced. It took me a very long time to realise where OFS was 
setting cache headers, and longer still to figure out how to customise it.

Compare that to what we're doing now in plone.caching: Get the published 
object from the request in a post-publication hook (IPubBeforeCommit 
mostly); look up a policy for that object based on its type; ask the 
policy the set headers and/or mutate the response in other ways. It's a 
lot easier to understand. And because it's centralised, there's a single 
type of policy object to implement for bespoke caching regardless of 
what you're trying to set headers for (e.g. views of content vs. 
downloadable files vs. stylesheets, etc.), and a single place to trace 
when caching headers are not acting as they should.

> No other code in Zope2 proper (or its dependent eggs) calls
> RESPONSE.write att all:
>
>   $ find src -name "*.py" | xargs grep -li "response\.write("
>   src/Testing/tests/test_makerequest.py
>   src/OFS/Image.py
>   $ find eggs -name "*.py" | xargs grep -li "response\.write("
>   $
>
> I'm -1 on adding the event without more justification.

I can think of a few other reasons:

  - We have events that allow people to hook in just before the response 
is sent back, either to mutate the response body, or to set headers, 
that work for every scenario except one edge case: OFS range responses. 
It makes sense to close that gap and provide sufficient hooks. I've got 
one use case above, but I'm sure there are others, too.

  - It doesn't do any harm. I'll take out the getSite() ugliness, so 
it's just a one-line notify(PubBeforeStreaming(self)) in the write() method.

  - Plone needs it. :-)

If it's really shot down, I'll monkey patch an event in, but that 
sounds pretty stupid. One of the main things we wanted to avoid with new 
caching effort was a ton of monkey patches (witness the amount of 
patches CacheFu juggles to get even a modicum of caching policy sanity).

Martin

-- 
Author of `Professional Plone Development`, a book for developers who
want to work with Plone. See http://martinaspeli.net/plone-book