You are viewing a plain text version of this content. The canonical link for it is here.
Posted to modperl@perl.apache.org by Steve Hay <st...@googlemail.com> on 2014/10/31 09:47:07 UTC
Re: A patch to fix one sort of mod_perl crash I've been experiencing.
On 4 June 2014 21:02, Kandarian, Richard M <ri...@lanl.gov> wrote:
> Reading MP_TRACE messages I noticed that for every modperl_interp_select() call during a request there were two modperl_interp_unselect() calls, one bracketing the _select() call and one from the registered cleanup when the pool was destroyed. The attached patch stopped those crashes. I would call my fix VERY tentative for various reasons including my noobiness and I've added comments near the active ingredient in modperl_interp.c. Inactive ingredients include diffs in the patch which are my adding trace info.
>
Apologies for the long delay in following up on this.
Where was the crash that you were seeing (the one which your patch
stops from happening)?
The patch makes modperl_interp_unselect() return early when the interp
in question is already not "in use", but that in itself won't stop a
crash from occurring -- there is no harm in turning off the "in use"
flag again when it is already off. Was it one of the other lines
further down modperl_interp_unselect() that crashed, or something
somewhere else?
As for returning early if refcnt is <= 0, I added an assertion that
refcnt is > 0, and didn't see it triggered... Having said that I also
didn't see the existing assertion that MpInterpIN_USE(interp) is true
get triggered either, but I did see some trace messages about the
interp not being in use, so maybe MP_ASSERT() isn't working as I
expected. (It is a debug build I'm using.)
And regarding setting refcnt to 0, that doesn't seem quite right, but
certainly decrementing it sounds to me like it should be done to match
the increment which is done in modperl_interp_pool_select() and
modperl_interp_select(). The end of the latter adds an extra refcnt
for the registered cleanup, so certainly something needs to be called
to decrement the refcnt twice -- we shouldn't return from
modperl_interp_unselect() so early that the refcnt decrement gets
skipped.
I haven't seen the crash that you were experiencing, which makes it
difficult to test things out. All the tests pass (using SVN trunk with
httpd-2.4.9 and perl-5.21.0) except for the four failures documented
in the README. I tried adding a decrement of refcnt to cover the case
where it doesn't already return early due to refcnt being > 1. This
decrement used to happen, but got removed by r1562772 and I'm not sure
if that was intentional. Tests still pass with this change:
Index: src/modules/perl/modperl_interp.c
===================================================================
--- src/modules/perl/modperl_interp.c (revision 1635423)
+++ src/modules/perl/modperl_interp.c (working copy)
@@ -273,7 +273,7 @@
modperl_interp_t *interp = (modperl_interp_t *)data;
modperl_interp_pool_t *mip = interp->mip;
- MP_ASSERT(interp && MpInterpIN_USE(interp));
+ MP_ASSERT(interp && MpInterpIN_USE(interp) && interp->refcnt > 0);
MP_TRACE_i(MP_FUNC, "unselect(interp=%pp): refcnt=%d",
interp, interp->refcnt);
@@ -285,6 +285,7 @@
}
MpInterpIN_USE_Off(interp);
+ --interp->refcnt;
modperl_thx_interp_set(interp->perl, NULL);
#ifdef MP_DEBUG
I also tried going further, along the lines of your own patch, but
again making sure that the refcnt decrement still happens even in the
case of both early returns:
Index: src/modules/perl/modperl_interp.c
===================================================================
--- src/modules/perl/modperl_interp.c (revision 1635423)
+++ src/modules/perl/modperl_interp.c (working copy)
@@ -273,17 +273,24 @@
modperl_interp_t *interp = (modperl_interp_t *)data;
modperl_interp_pool_t *mip = interp->mip;
- MP_ASSERT(interp && MpInterpIN_USE(interp));
+ MP_ASSERT(interp && MpInterpIN_USE(interp) && interp->refcnt > 0);
MP_TRACE_i(MP_FUNC, "unselect(interp=%pp): refcnt=%d",
interp, interp->refcnt);
- if (interp->refcnt > 1) {
- --interp->refcnt;
+ --interp->refcnt;
+
+ if (interp->refcnt > 0) {
MP_TRACE_i(MP_FUNC, "interp=0x%lx, refcnt=%d -- interp still in use",
(unsigned long)interp, interp->refcnt);
return APR_SUCCESS;
}
+ if (!MpInterpIN_USE(interp)){
+ MP_TRACE_i(MP_FUNC, "interp=0x%lx, refcnt=%d -- interp not in use, alre
ady unselected?",
+ (unsigned long)interp, interp->refcnt);
+ return APR_SUCCESS;
+ }
+
MpInterpIN_USE_Off(interp);
modperl_thx_interp_set(interp->perl, NULL);
@@ -301,6 +308,9 @@
interp, mip->tipool->size, mip->tipool->in_use);
}
+ MP_TRACE_i(MP_FUNC, "interp=0x%lx, refcnt=%d -- interp now unselected",
+ (unsigned long)interp, interp->refcnt);
+
return APR_SUCCESS;
}
Tests still pass, but I'm shooting in the dark without knowing where
the crash was.
It does seem to me like the refcnt decrement needs doing, though. Does
that change alone (i.e. the first of my diffs above) fix your crash,
or do you still need the early return too (i.e. the second of my diffs
above)?
I will ask Jan Kaluza about the refcnt business since he authored the
change I cited above which dropped the refcnt decrement in the case
where refcnt is 1.
Re: A patch to fix one sort of mod_perl crash I've been experiencing.
Posted by Steve Hay <st...@googlemail.com>.
On 31 October 2014 08:47, Steve Hay <st...@googlemail.com> wrote:
> On 4 June 2014 21:02, Kandarian, Richard M <ri...@lanl.gov> wrote:
>> Reading MP_TRACE messages I noticed that for every modperl_interp_select() call during a request there were two modperl_interp_unselect() calls, one bracketing the _select() call and one from the registered cleanup when the pool was destroyed. The attached patch stopped those crashes. I would call my fix VERY tentative for various reasons including my noobiness and I've added comments near the active ingredient in modperl_interp.c. Inactive ingredients include diffs in the patch which are my adding trace info.
>>
>
> Apologies for the long delay in following up on this.
>
[...]
>
> I will ask Jan Kaluza about the refcnt business since he authored the
> change I cited above which dropped the refcnt decrement in the case
> where refcnt is 1.
After speaking with Jan, I have now committed the second patch above
in r1636289.
(We think that the refcnt change alone should fix your crash, but it
doesn't hurt to have the extra safety net of the early return if the
interp is already unselected, although that shouldn't happen now.)
Many thanks for your work on this.