Mailing List Archive: Apache: Dev

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c

Index | Next | Previous | View Threaded

rpluem at apache

Mar 11, 2010, 9:57 PM

Post #1 of 7 (1240 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c

On 10.03.2010 16:00, sf [at] apache wrote:
> Author: sf
> Date: Wed Mar 10 15:00:50 2010
> New Revision: 921378
>
> URL: http://svn.apache.org/viewvc?rev=921378&view=rev
> Log:
> Move initialization to process_connection hook, right before
> ap_process_http_request. This ensures that we are not inserted for other
> protocol handlers (like mod_ftp) and mod_proxy's backend connections.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
>

> Modified: httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_reqtimeout.c?rev=921378&r1=921377&r2=921378&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_reqtimeout.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_reqtimeout.c Wed Mar 10 15:00:50 2010

> if (ccfg->in_keep_alive) {
> /* For this read, the normal keep-alive timeout must be used */
> @@ -114,6 +105,24 @@ static apr_status_t reqtimeout_filter(ap
> return ap_get_brigade(f->next, bb, mode, block, readbytes);
> }
>
> + if (!ccfg->socket) {
> + core_net_rec *net_rec;
> + ap_filter_t *core_in = f->next;
> +
> + while (core_in && core_in->frec != ap_core_input_filter_handle)
> + core_in = core_in->next;
> +
> + if (!core_in) {
> + ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, f->c,
> + "mod_reqtimeout: Can't get socket "
> + "handle from core_input_filter");
> + ap_remove_input_filter(f);
> + return ap_get_brigade(f->next, bb, mode, block, readbytes);
> + }
> + net_rec = core_in->ctx;
> + ccfg->socket = net_rec->client_socket;
> + }
> +

Hm, this looks kind of ugly. Why not leaving things in
r->connection->conn_config and run through both hooks (pre_connection and
process_connection). The first one stores the socket, the second one applies
the filter if ever reached.

Regards

Rüdiger

sf at sfritsch

Mar 12, 2010, 11:47 AM

Post #2 of 7 (1171 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On Fri, 12 Mar 2010, Ruediger Pluem wrote:
>> @@ -114,6 +105,24 @@ static apr_status_t reqtimeout_filter(ap
>> return ap_get_brigade(f->next, bb, mode, block, readbytes);
>> }
>>
>> + if (!ccfg->socket) {
>> + core_net_rec *net_rec;
>> + ap_filter_t *core_in = f->next;
>> +
>> + while (core_in && core_in->frec != ap_core_input_filter_handle)
>> + core_in = core_in->next;
>> +
>> + if (!core_in) {
>> + ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, f->c,
>> + "mod_reqtimeout: Can't get socket "
>> + "handle from core_input_filter");
>> + ap_remove_input_filter(f);
>> + return ap_get_brigade(f->next, bb, mode, block, readbytes);
>> + }
>> + net_rec = core_in->ctx;
>> + ccfg->socket = net_rec->client_socket;
>> + }
>> +
>
> Hm, this looks kind of ugly. Why not leaving things in
> r->connection->conn_config and run through both hooks (pre_connection and
> process_connection). The first one stores the socket, the second one applies
> the filter if ever reached.

I wanted to avoid allocating the memory for mod_reqtimeout's conn_config
if it is not enabled anyway. Without the conn_config, there is no place to
store the socket. If you think wasting a bit of memory is better than that
loop, I am fine with that too. But usually there are only very few
connection input filters and the chain we have to traverse is rather
short. Actually I don't know of any module that sits between
mod_reqtimeout and the core_input_filter, even mod_ssl is behind
mod_reqtimeout.

I have also tried inserting the filter in pre_connection and storing the
socket as filter context, and then removing the filter in the first
invocation of reqtimeout_filter. But this did not work well either,
because ap_remove_input_filter can't remove a connection filter from the
current request (is this a bug?).

rpluem at apache

Mar 15, 2010, 12:28 AM

Post #3 of 7 (1127 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On 12.03.2010 20:47, Stefan Fritsch wrote:
> On Fri, 12 Mar 2010, Ruediger Pluem wrote:
>>> @@ -114,6 +105,24 @@ static apr_status_t reqtimeout_filter(ap
>>> return ap_get_brigade(f->next, bb, mode, block, readbytes);
>>> }
>>>
>>> + if (!ccfg->socket) {
>>> + core_net_rec *net_rec;
>>> + ap_filter_t *core_in = f->next;
>>> +
>>> + while (core_in && core_in->frec != ap_core_input_filter_handle)
>>> + core_in = core_in->next;
>>> +
>>> + if (!core_in) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, f->c,
>>> + "mod_reqtimeout: Can't get socket "
>>> + "handle from core_input_filter");
>>> + ap_remove_input_filter(f);
>>> + return ap_get_brigade(f->next, bb, mode, block, readbytes);
>>> + }
>>> + net_rec = core_in->ctx;
>>> + ccfg->socket = net_rec->client_socket;
>>> + }
>>> +
>>
>> Hm, this looks kind of ugly. Why not leaving things in
>> r->connection->conn_config and run through both hooks (pre_connection and
>> process_connection). The first one stores the socket, the second one
>> applies
>> the filter if ever reached.
>
> I wanted to avoid allocating the memory for mod_reqtimeout's conn_config
> if it is not enabled anyway. Without the conn_config, there is no place
> to store the socket. If you think wasting a bit of memory is better than
> that loop, I am fine with that too. But usually there are only very few

I think wasting this small amount of memory is ok and if someone does not
want mod_reqtimeout he should simply not load it and in this case there
is no waste at all.

> connection input filters and the chain we have to traverse is rather
> short. Actually I don't know of any module that sits between
> mod_reqtimeout and the core_input_filter, even mod_ssl is behind
> mod_reqtimeout.
>
> I have also tried inserting the filter in pre_connection and storing the
> socket as filter context, and then removing the filter in the first
> invocation of reqtimeout_filter. But this did not work well either,
> because ap_remove_input_filter can't remove a connection filter from the
> current request (is this a bug?).

Hm, that sounds like a bug, but no time to dig deeper.

Regards

Rüdiger

jorton at redhat

Mar 15, 2010, 3:53 AM

Post #4 of 7 (1130 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On Fri, Mar 12, 2010 at 08:47:48PM +0100, Stefan Fritsch wrote:
> I have also tried inserting the filter in pre_connection and storing
> the socket as filter context, and then removing the filter in the
> first invocation of reqtimeout_filter. But this did not work well
> either, because ap_remove_input_filter can't remove a connection
> filter from the current request (is this a bug?).

Sounds like this issue:

http://marc.info/?l=apache-httpd-dev&m=105366252619530&w=2

though thinking about it now, I wonder if this has been made more severe
by this change:

http://svn.apache.org/viewvc?rev=600473&view=rev

since all conn-level filters now have no knowledge of a request_rec even
if they were created with such knowledge.

Regards, Joe

sf at sfritsch

Mar 15, 2010, 1:28 PM

Post #5 of 7 (1119 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On Mon, 15 Mar 2010, Ruediger Pluem wrote:
>>>> @@ -114,6 +105,24 @@ static apr_status_t reqtimeout_filter(ap
>>>> return ap_get_brigade(f->next, bb, mode, block, readbytes);
>>>> }
>>>>
>>>> + if (!ccfg->socket) {
>>>> + core_net_rec *net_rec;
>>>> + ap_filter_t *core_in = f->next;
>>>> +
>>>> + while (core_in && core_in->frec != ap_core_input_filter_handle)
>>>> + core_in = core_in->next;
>>>> +
>>>> + if (!core_in) {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, f->c,
>>>> + "mod_reqtimeout: Can't get socket "
>>>> + "handle from core_input_filter");
>>>> + ap_remove_input_filter(f);
>>>> + return ap_get_brigade(f->next, bb, mode, block, readbytes);
>>>> + }
>>>> + net_rec = core_in->ctx;
>>>> + ccfg->socket = net_rec->client_socket;
>>>> + }
>>>> +
>>>
>>> Hm, this looks kind of ugly. Why not leaving things in
>>> r->connection->conn_config and run through both hooks (pre_connection and
>>> process_connection). The first one stores the socket, the second one
>>> applies
>>> the filter if ever reached.
>>
>> I wanted to avoid allocating the memory for mod_reqtimeout's conn_config
>> if it is not enabled anyway. Without the conn_config, there is no place
>> to store the socket. If you think wasting a bit of memory is better than
>> that loop, I am fine with that too. But usually there are only very few
>
> I think wasting this small amount of memory is ok and if someone does not
> want mod_reqtimeout he should simply not load it and in this case there
> is no waste at all.

Found a simpler way:

if (!ccfg->socket) {
ccfg->socket = ap_get_module_config(f->c->conn_config, &core_module);
}

Is that OK or are other modules not supposed to use core_module?

Cheers,
Stefan

sf at sfritsch

Mar 15, 2010, 1:35 PM

Post #6 of 7 (1116 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On Mon, 15 Mar 2010, Joe Orton wrote:

> On Fri, Mar 12, 2010 at 08:47:48PM +0100, Stefan Fritsch wrote:
>> I have also tried inserting the filter in pre_connection and storing
>> the socket as filter context, and then removing the filter in the
>> first invocation of reqtimeout_filter. But this did not work well
>> either, because ap_remove_input_filter can't remove a connection
>> filter from the current request (is this a bug?).
>
> Sounds like this issue:
>
> http://marc.info/?l=apache-httpd-dev&m=105366252619530&w=2

Yes, that's it. So it's just the first connection filter that cannot be
removed.

> though thinking about it now, I wonder if this has been made more severe
> by this change:
>
> http://svn.apache.org/viewvc?rev=600473&view=rev
>
> since all conn-level filters now have no knowledge of a request_rec even
> if they were created with such knowledge.

For mod_reqtimeout it doesn't make a difference as the request_rec was
never available. The only fixes I can think of are creating a dummy filter
between the connection filters and the per-request filters (as outlined in
the mail linked to above), or adding some API that allows
ap_remove_input_filter() to get the request_rec from the conn_rec. I am
not sure if the latter may create races in async threaded MPMs, though.

In any case, I think I have worked around the problem for mod_reqtimeout.

sf at sfritsch

Mar 15, 2010, 1:47 PM

Post #7 of 7 (1115 views)
Permalink

Re: svn commit: r921378 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_reqtimeout.c [In reply to]

On Monday 15 March 2010, Stefan Fritsch wrote:
> Found a simpler way:
>
> if (!ccfg->socket) {
> ccfg->socket = ap_get_module_config(f->c->conn_config,
> &core_module); }
>
> Is that OK or are other modules not supposed to use core_module?

Commited. Other modules do it that way, too.

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads