httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r729538 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_proxy_scgi.xml modules/mappers/mod_rewrite.c modules/proxy/config.m4 modules/proxy/mod_proxy_scgi.c
Date Sat, 27 Dec 2008 19:40:11 GMT


On 12/27/2008 07:27 PM, André Malo wrote:
> * Ruediger Pluem wrote:
> 
>> On 12/27/2008 04:07 PM, André Malo wrote:
>>> * Ruediger Pluem wrote:
>>>> On 12/26/2008 10:41 PM, nd@apache.org wrote:
>>>>> URL: http://svn.apache.org/viewvc?rev=729538&view=rev
>>>> Hm. Why this rather complex approach with the request_status hook?
>>>> Why not doing the subrequest here or even better just inserting the
>>>> file buckets into the brigade right here and be done?
>>> The whole point of the sendfile stuff is to free the backend socket as
>>> early as possible and leave the rest to the apache. So I'm going out of
>>> the proxy loop and handle everything in the request status hook, which
>>> runs after the cleanup.
>> For freeing the backend socket it is IMHO possible to call
>> ap_proxy_release_connection first and do the subrequest run afterwards in
>> scgi_handler. This avoids the need for using the request_status hook
>> which makes the code IMHO more complex then needed.
> 
> Hmm. But I still have the internal redirect code there as well. I'd rather 
> leave that as-is for the sake of separating concerns. If you ask me, I 
> think, it's more simple now ;-)

IMHO this could be moved to the same location as well :-).

> 
>>> Also I don't want to duplicate the file delivery logic from core
>>> (including sendfile, mmap etc), so putting file buckets myself into the
>>> stream sounds not like the way to go.
>> Hm, there isn't that much basic logic to duplicate, basicly
>>
>>             e = apr_brigade_insert_file(bb, fd, 0, r->finfo.size,
>> r->pool);
>>
>> #if APR_HAS_MMAP
>>             if (d->enable_mmap == ENABLE_MMAP_OFF) {
>>                 (void)apr_bucket_file_enable_mmap(e, 0);
>>             }
>> #endif
> 
> Isn't that private config?

At least it is one of another module. And yes I agree if there is no official
API to get the setting of d->enable_mmap (currently too lazy to check this) it
is the best to stay with the subrequest approach.

Regards

Rüdiger


Mime
View raw message