What can I do ...

to have these 2 patches applied to trunk or to get a
statement why they cannot 
be applied?

http://www.gossamer-threads.com/lists/modperl/dev/93415
http://www.gossamer-threads.com/lists/modperl/dev/93466

Torsten

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribeperl.apache.org
For additional commands, e-mail: dev-helpperl.apache.org

Torsten Foertsch wrote:
> to have these 2 patches applied to trunk or to get a
statement why they cannot 
> be applied?
> 
> http://www.gossamer-threads.com/lists/modperl/dev/93415
> http://www.gossamer-threads.com/lists/modperl/dev/93466

I have been following this somewhat, but not grokking all of
it.

Can you write a test that shows the change and its effect?

I was poking around that area of the code for the stacked
handlers bug 
so I understand some of it but a test would be really be
helpful here.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribeperl.apache.org
For additional commands, e-mail: dev-helpperl.apache.org

Hi Fred,

On Wednesday 09 May 2007 10:42, Fred Moyer wrote:
> I have been following this somewhat, but not grokking
all of it.
>
> Can you write a test that shows the change and its
effect?
>
> I was poking around that area of the code for the
stacked handlers bug
> so I understand some of it but a test would be really
be helpful here.

Well, I have thought of that myself but I was not able to
produce a simple 
test case. It is simple to get the memory corrupted (just
call 
$r->push_handlers twice for the same phase) but it's hard
to predict when the 
segfault will occur since it depends completely on your
memory mgmt 
implementation.

But I can explain the bug more thoroughly.

modperl_handler_lookup_handlers is declared this way:

MpAV **modperl_handler_lookup_handlers(modperl_config_dir_t
*dcfg,
                                       modperl_config_srv_t
*scfg,
                                       modperl_config_req_t
*rcfg,
                                       apr_pool_t *p,
                                       int type, int idx,
                                      
modperl_handler_action_e action,
                                       const char **desc)

It is called at startup to configure handlers in
server/per_dir config and at 
request time to configure handlers in request config.

At startup when there is no request it is called with
rcfg==0. p is in this 
case a longer living pool.

At request time it is passed rcfg!=0 and the request pool in
p.

The function initializes avp=0 and ravp=0.

Then it says

        avp = &dcfg->handlers_per_dir[idx];
        if (rcfg) {
            ravp = &rcfg->handlers_per_dir[idx];
        }

(The scfg->handlers_per_srv case is similar)

So, avp now points to an array element inside dcfg while
ravp at request time 
points to an array element inside rcfg. At startup it
remains 0.

Later the function ensures that avp!=0 and then does this

        if (ravp && !*ravp) {
            if (*avp) {
                /* merge with existing configured handlers
*/
                *ravp = apr_array_copy(p, *avp);
            }
            else {
                /* no request handlers have been previously
pushed or set */
                *ravp = modperl_handler_array_new(p);
            }
        }
        else if (!*avp) {
            /* directly modify the configuration at startup
time */
            *avp = modperl_handler_array_new(p);
        }

The arrays dcfg->handlers_per_dir and
rcfg->handlers_per_dir are initially set 
to all NULL pointers. If an element is 0 it means there is
no handler set for 
this phase. Otherwise it points to an apr_array.

So, at startup time we have to make sure that *avp points to
an apr_array and 
return that while at request time *ravp should point to an
apr_array and that 
should be returned.

Let's first look what happens at startup. rcfg==NULL and
hence ravp==NULL. 
The "else if" checks whether the appropriate
element of dcfg is already 
initialized. If not it is assigned an empty apr_array. Here
the program works 
correct.

Now at request time. rcfg!=NULL and hence ravp!=NULL. *ravp
may be NULL if 
there is no handler set yet.

The "if (ravp && !*ravp)" works correct if
*ravp is NULL. In this case (the 
first time $r->push_handlers is called) *ravp gets
initialized.

But if the code is run a second time the if condition is
false because *ravp 
is already set. In this case we must do nothing but simply
return ravp.

So we hit the else if branch. If now *avp is NULL we
initialize it. And this 
is wrong. To cite from Nick Kew book:

  When processing a request, use the fields of the
request_rec - in
  particular, the request pool and the request configuration
vector.
  Treat everything else as read-only.

In our case we modify a structure which is valid until
server shutdown and 
write there a pointer that is allocated from the request
pool since p in

            *avp = modperl_handler_array_new(p);

is the request pool.

So how can it be that *avp is NULL at request time? That
simply means there 
has been no handler configured at startup time.

What possible solutions are there? Let's have a look at the
code for the other

      case MP_HANDLER_ACTION_SET:

Here it says

        if (ravp) {
        }
        else if (*avp) {
        }
        else {
        }

This is correct because no else branch is entered if
ravp!=NULL.

For the ACTION_PUSH case we can do it the same way:

         if (ravp) {
            if( !*ravp ) {
                if (*avp) {
                    /* merge with existing configured
handlers */
                    *ravp = apr_array_copy(p, *avp);
                }
                else {
                    /* no request handlers have been
previously pushed or set 
*/
                    *ravp = modperl_handler_array_new(p);
                }
            }
        }
        else if (!*avp) {
            /* directly modify the configuration at startup
time */
            *avp = modperl_handler_array_new(p);
        }

or the way the original patch did:

        if (ravp && !*ravp) {
        }
        else if (ravp) {
	    /* ravp is already initialized: do nothing */
        }
        else if (!*avp) {
        }

So the bug reduces to 

if (a) {
  if (b) {
  }
} else if (c) {
}

not being the same as 

if (a && b) {
} else if (c) {
}

Hope that helps,
Torsten

On Wednesday 09 May 2007 10:42, Fred Moyer wrote:
> > to have these 2 patches applied to trunk or to get
a statement why they
> > cannot be applied?
> >
> > http://www.gossamer-threads.com/lists/modperl/dev/93415
> > http://www.gossamer-threads.com/lists/modperl/dev/93466
>
> I have been following this somewhat, but not grokking
all of it.
>
> Can you write a test that shows the change and its
effect?

So, this patch adds an extra test to show the effect of the
first patch. With 
trunk the handling thread is locked forever after the 2nd
test. If the number 
of interpreters is limited to 1 it is locked after the 1st
request.

With the patch applied all works smoothly.

Torsten

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribeperl.apache.org
For additional commands, e-mail: dev-helpperl.apache.org

Torsten Foertsch wrote:
> to have these 2 patches applied to trunk or to get a
statement why they cannot 
> be applied?

Something very similar to what you've been doing already
 In
general,
that's the correct approach. Post patches, explanations,
test cases that
expose the problem, and generally poke people about it.

> http://www.gossamer-threads.com/lists/modperl/dev/93415

This one looks pretty good, and I think it's a cleaner
approach
to the currently uncorrect one.

> http://www.gossamer-threads.com/lists/modperl/dev/93466

Also a good patch, with just a few style nits.

Sorry for the lag in responding, but like a lot of us, we've
got
dayjobs that eat up our hacking time.

I've seen your patches come in, and I've read thru them
quite
carefully, rest assured. They have been at the top of my
mod_perl
queue for some time now, but the difficult part is verifying
they
are correct without a reproducible test case.

I'll see about getting these sorted out this week-end.

Thanks again!

------------------------------------------------------------
------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680
1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.or
g/       m/gozer(apache|cpan|ectoplasm).org/

Philippe M. Chiasson wrote:
> Torsten Foertsch wrote:
>> to have these 2 patches applied to trunk or to get
a statement why they cannot 
>> be applied?
> 
> Something very similar to what you've been doing
already  In
general,
> that's the correct approach. Post patches,
explanations, test cases that
> expose the problem, and generally poke people about
it.
> 
>> http://www.gossamer-threads.com/lists/modperl/dev/93415
> 
> This one looks pretty good, and I think it's a cleaner
approach
> to the currently uncorrect one.

I've just spent the evening looking at this more in details,
and the
more I look, the more things look significantly broken. My
latest
feeling was that your patch was not actually adressing the
underlying
problem.

Here is where I am at so far. In many cases, once an
interpreter has been
acquired and not yet released, nothing should really need to
call interp_select()
to find an interpreter, as opposed to being handed that
interpreter directly.

On the topic of interp->refcnt, check out
xs/APR/Pool/APR_Pool.h, it's
the current customer of it, and uses it again in a
strange/unobvious
way.

Anyways, I'll be poking at this some more shortly and post
more of my
findings.

Torsten, thanks for the ithreads3 test case, it does
correctly exercice
the problems every time. (I'd like to include it, but it
would need to
be somewhat cleaned up to follow our current coding
conventions, apart
from that, pretty good).

Good night!

------------------------------------------------------------
------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680
1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.or
g/       m/gozer(apache|cpan|ectoplasm).org/

On Monday 14 May 2007 08:35, Philippe M. Chiasson wrote:
> >> http://www.gossamer-threads.com/lists/modperl/dev/93415
> >
> > This one looks pretty good, and I think it's a
cleaner approach
> > to the currently uncorrect one.
>
> I've just spent the evening looking at this more in
details, and the
> more I look, the more things look significantly broken.
My latest
> feeling was that your patch was not actually adressing
the underlying
> problem.

I was there myself, see 
http://www.gossamer-threads.com/lists/modperl/dev/9
2674#92674

> Here is where I am at so far. In many cases, once an
interpreter has been
> acquired and not yet released, nothing should really
need to call
> interp_select() to find an interpreter, as opposed to
being handed that
> interpreter directly.

The point is you can go your way and prevent recursive
calls. That'd mean 
redesign of the modperl_response_handler* functions at
least. Or you can make 
modperl_interp_unselect reenterable and handle the
recursion. That is what I 
did. I think refcounting is a quite good approach to that. I
see 
interp_select() as the interface that hands you the *right*
interpreter. It 
handles all the details of which interp and where it comes
from. The thing 
that disturbs me a bit is the PUTBACK flag and its
inspection all over the 
code. I think it would be cleaner to call select/unselect
each time and let 
them decide what to do. And what disturbs me even more is
the way 
select/unselect is called, sometimes though a macro,
sometimes directly, 
sometimes conditionally ("if(c||r)" or so). It was
very hard to figure out 
what was going on.

The semantics of PerlCleanupHandler is a bit strange in case
of scope==handler 
but I think it is quite good the way it is. It needs to be
explained as a way 
to deallocate resources that are bound to a certain perl
instance while a 
pool cleanup handler handles resources that are bound to the

request/connection/process whatever the pool belongs to.

> On the topic of interp->refcnt, check out
xs/APR/Pool/APR_Pool.h, it's
> the current customer of it, and uses it again in a
strange/unobvious
> way.
>
> Anyways, I'll be poking at this some more shortly and
post more of my
> findings.

I really like the idea of refcounting. Initially I was
looking for a way to 
allocate an interp from trans to fixup and release it just
before response. 
This way most of the slow network IO is made without an
interpreter. But it 
preserves the possibility to communicate via pnotes between
the phases.

With my patch and the way APR::Pool handles refcounts it can
be done very 
simple:

in trans:
$r->pnotes->=$r->pool->new;

this preserves the interp from being freed.

Then in fixup:
delete $r->pnotes->;

And since the interp is stored in rcfg it is also the faster
way compared with 
a scope==request. With scope==request||subrequest the interp
is stored in the 
request pools userdata. That means a hash lookup instead of
a simple 
structure access. It would be even faster if that initial
subpool creation 
could be avoided. For that I think a Apache2::Interp or
Modperl::Interp 
module would be good to access the refcnt. Would something
like that be 
accepted? What would be the right name?

I'd vote for something like rcfg->interp but not in rcfg
but in a connection 
bound structure to always store the interp currently in use.
Then decisions 
about the lifetime of that field are made completely in
interp_(un)select(). 
This pair of functions is then called every time an interp
is needed. For the 
scope==subreq case there must be something special (an
apr_array) then I 
think.

Another strangeness is that PerlInterpScope is of directory
scope. What to do 
when there is a PerlTransHandler configured with
scope==handler but in 
MapToStorage a PerlInterpScope directive says it is
scope==subreq or so. This 
needs to be investigated I think.

> Torsten, thanks for the ithreads3 test case, it does
correctly exercice
> the problems every time. (I'd like to include it, but
it would need to
> be somewhat cleaned up to follow our current coding
conventions, apart
>
> >from that, pretty good).

This weekend I made a patch to allow APR::ThreadRWLock by
now without test. 
I'll see if it does what I expect it to do and then send it
to the list.

Torsten