Hanno Schlichting wrote:

On Sun, Mar 21, 2010 at 10:07 AM, Martin Aspeli <optilude+lists@gmail.com> wrote:

...

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.

Why would you need to have the event on the request, if all you want is to set headers? Why not make it an event with the response as the argument instead?

Mainly because that's what all the other IPub* events carry with them. We discovered this omission implementing plone.caching, which only needs the response, but it's not all that unlikely that something else may need the request. Of course, "something else" could do the same getSite() trick if needed, or use five.globalrequest. Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book

Hanno Schlichting wrote:

On Sun, Mar 21, 2010 at 12:05 PM, Martin Aspeli <optilude+lists@gmail.com> wrote:

...

Hanno Schlichting wrote:

...

Why would you need to have the event on the request, if all you want is to set headers? Why not make it an event with the response as the argument instead? Mainly because that's what all the other IPub* events carry with them. We discovered this omission implementing plone.caching, which only needs the response, but it's not all that unlikely that something else may need the request.

At the point where you start out writing the actual response byte-stream, I think it's too late to do anything useful with the request.

But consistency is good, so maybe the different nature of the event could be reflected in the event name more clearly.

...

Of course, "something else" could do the same getSite() trick if needed, or use five.globalrequest.

Yes, please. Unless there's a real need for such a hack, I'd rather not see it in Zope2.

Would you be happy with the approach outlined if the event carried just the response instead? Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book

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