You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Torsten Foertsch <to...@gmx.net> on 2007/05/09 10:12:23 UTC
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-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
explaining the dcfg bug (was: What can I do ...)
Posted by Torsten Foertsch <to...@gmx.net>.
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
Re: What can I do ...
Posted by Torsten Foertsch <to...@gmx.net>.
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
Re: What can I do ...
Posted by Fred Moyer <fr...@taperfriendlymusic.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-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: What can I do ...
Posted by Torsten Foertsch <to...@gmx.net>.
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/92674#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->{my_pool}=$r->pool->new;
this preserves the interp from being freed.
Then in fixup:
delete $r->pnotes->{my_pool};
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
Re: What can I do ...
Posted by "Philippe M. Chiasson" <go...@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.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/
Re: What can I do ...
Posted by "Philippe M. Chiasson" <go...@ectoplasm.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.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/