Re: svn commit: r1676417 - /perl/modperl/trunk/src/modules/perl/modperl_interp.c

On 05/28/2015 12:44 AM, Steve Hay wrote:
>
> On 25 May 2015 10:11, "Jan Kaluža" <[email protected]
> <mailto:[email protected]>> wrote:
>  >
>  > On 05/24/2015 04:50 PM, Steve Hay wrote:
>  >>
>  >> On 20 May 2015 at 14:51, Jan Kaluža <[email protected]
> <mailto:[email protected]>> wrote:
>  >>>
>  >>> On 05/20/2015 11:49 AM, Jan Kaluža wrote:
>  >>>>
>  >>>>
>  >>>> On 05/15/2015 12:26 PM, Jan Kaluža wrote:
>  >>>>>
>  >>>>>
>  >>>>> On 05/15/2015 11:57 AM, Jan Kaluža wrote:
>  >>>>>>
>  >>>>>>
>  >>>>>> On 05/15/2015 10:01 AM, Steve Hay wrote:
>  >>>>>>>
>  >>>>>>>
>  >>>>>>> On 15 May 2015 at 08:56, Steve Hay <[email protected]
> <mailto:[email protected]>> wrote:
>  >>>>>>>>
>  >>>>>>>>
>  >>>>>>>> On 15 May 2015 at 07:14, Jan Kaluža <[email protected]
> <mailto:[email protected]>> wrote:
>  >>>>>>>>>
>  >>>>>>>>>
>  >>>>>>>>> On 05/14/2015 07:42 PM, Steve Hay wrote:
>  >>>>>>>>>>
>  >>>>>>>>>>
>  >>>>>>>>>>
>  >>>>>>>>>> On 14 May 2015 at 12:48, Jan Kaluža <[email protected]
> <mailto:[email protected]>> wrote:
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>> On 05/14/2015 11:24 AM, Niko Tyni wrote:
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>> On Sun, May 10, 2015 at 01:47:19PM +0100, Steve Hay wrote:
>  >>>>>>>>>>>>>
>  >>>>>>>>>>>>>
>  >>>>>>>>>>>>>
>  >>>>>>>>>>>>>
>  >>>>>>>>>>>>> On 28 April 2015 at 07:51,  <[email protected]
> <mailto:[email protected]>> wrote:
>  >>>>>>>>>>>>>>
>  >>>>>>>>>>>>>>
>  >>>>>>>>>>>>>>
>  >>>>>>>>>>>>>>
>  >>>>>>>>>>>>>> Author: jkaluza
>  >>>>>>>>>>>>>> Date: Tue Apr 28 06:51:12 2015
>  >>>>>>>>>>>>>> New Revision: 1676417
>  >>>>>>>>>>>>>>
>  >>>>>>>>>>>>>> URL: http://svn.apache.org/r1676417
>  >>>>>>>>>>>>>> Log:
>  >>>>>>>>>>>>>> Initialize interp->refcnt to 1 in modperl_interp_select.
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>
>  >>>>>>>>>>>>> I cannot understand why, but since this patch was applied
> I find
>  >>>>>>>>>>>>> that
>  >>>>>>>>>>>>> t\modules\proxy.t fails every time when I run the full "nmake
>  >>>>>>>>>>>>> test",
>  >>>>>>>>>>>>> but it always succeeds when I run it in isolation so I'm at a
>  >>>>>>>>>>>>> loss to
>  >>>>>>>>>>>>> find out what is going wrong. All other tests (apart from
> those
>  >>>>>>>>>>>>> known
>  >>>>>>>>>>>>> Win32-specific failures documented in README) still pass.
>  >>>>>>>>>>>>> Reverting
>  >>>>>>>>>>>>> the patch "fixes" the proxy.t problem, but probably isn't the
>  >>>>>>>>>>>>> right
>  >>>>>>>>>>>>> solution.
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>>
>  >>>>>>>>>>> It's caused by Perl_croak/modperl_croak.
>  >>>>>>>>>>>
>  >>>>>>>>>>> Lets take modperl_run_filter as an example. When following
>  >>>>>>>>>>> code-path is
>  >>>>>>>>>>> executed ...
>  >>>>>>>>>>>
>  >>>>>>>>>>>                    modperl_croak(aTHX_ MODPERL_FILTER_ERROR,
>  >>>>>>>>>>>                                  "a filter calling $f->read "
>  >>>>>>>>>>>                                  "must return OK and not
> DECLINED");
>  >>>>>>>>>>>
>  >>>>>>>>>>> ... the MP_INTERP_PUTBACK is not reached for some reason (I
>  >>>>>>>>>>> presume it's
>  >>>>>>>>>>> because of Perl_croak, but I don't understand why it stops the
>  >>>>>>>>>>> execution
>  >>>>>>>>>>> of
>  >>>>>>>>>>> the rest of modperl_run_filter method).
>  >>>>>>>>>>>
>  >>>>>>>>>>> Because of that, the interp->refcnt is not decreased, and the
>  >>>>>>>>>>> interp is
>  >>>>>>>>>>> not
>  >>>>>>>>>>> freed.
>  >>>>>>>>>>>
>  >>>>>>>>>>> I has been able to "fix" it by attached patch, but I would
> like to
>  >>>>>>>>>>> discuss
>  >>>>>>>>>>> more generic way how to fix that problem...
>  >>>>>>>>>>>
>  >>>>>>>>>>> Any ideas?
>  >>>>>>>>>>>
>  >>>>>>>>>>
>  >>>>>>>>>> modperl_croak() calls Perl_croak(), which is an XS interface to
>  >>>>>>>>>> Perl's
>  >>>>>>>>>> die() function, so surely you wouldn't expect anything
> immediately
>  >>>>>>>>>> after it to be run?
>  >>>>>>>>>>
>  >>>>>>>>>> I'm not sure exactly where it does end up, though. It must be
>  >>>>>>>>>> getting
>  >>>>>>>>>> caught by some eval somewhere since we aren't exiting the
> process,
>  >>>>>>>>>> but
>  >>>>>>>>>> presumably it wouldn't be possible to do appropriate clean-up
>  >>>>>>>>>> wherever
>  >>>>>>>>>> it lands up unless there is some mechanism for registering
> required
>  >>>>>>>>>> clean-up behaviour? Otherwise maybe we need to pass interp into
>  >>>>>>>>>> modperl_croak(), or into a new version of that if not all cases
>  >>>>>>>>>> require it, so that it can do the MP_INTERP_PUTBACK(interp,
> aTHX)
>  >>>>>>>>>> call?
>  >>>>>>>>>>
>  >>>>>>>>>
>  >>>>>>>>> What worries me here a bit is that we would have to
>  >>>>>>>>> MP_INTEPR_PUTBACK the
>  >>>>>>>>> PerlInterp which is later used for PerlCroak, if I understand it
>  >>>>>>>>> right.
>  >>>>>>>>>
>  >>>>>>>>> I have found out that usually when modperl_croak is called, the
>  >>>>>>>>> refcnt of
>  >>>>>>>>> the interp is above 1, so it wouldn't get freed prematurely, but
>  >>>>>>>>> still.
>  >>>>>>>>>
>  >>>>>>>>> I think for now we should putback the interp only when
>  >>>>>>>>> interp->refcnt > 1,
>  >>>>>>>>> it wouldn't fully fix all bugs, but lot of them would be fixed by
>  >>>>>>>>> that.
>  >>>>>>>>>
>  >>>>>>>>> If someone knows how Perl_croak works and if it's possible to
>  >>>>>>>>> cleanup the
>  >>>>>>>>> interp after that, it would be great to share that info .
>  >>>>>>>>>
>  >>>>>>>>
>  >>>>>>>> My understanding of Perl_croak() is that it either exits the
> process
>  >>>>>>>> (if not inside an eval()) or else calls the system's
> longjmp(), which
>  >>>>>>>> resumes execution from immediately after where the corresponding
>  >>>>>>>> setjmp() was called, having restored the process environment
> to the
>  >>>>>>>> original state at that point too.
>  >>>>>>>>
>  >>>>>>>> In the perl source, the setjmp()/longjmp() of eval()/die() are
> done by
>  >>>>>>>> the JMPENV_PUSH in Perl_eval_sv() (maybe called from
> Perl_eval_pv())
>  >>>>>>>> and the JMPENV_JUMP in Perl_die_unwind(), called from
> Perl_vcroak().
>  >>>>>>>> The JMPENV* macros are in cop.h, and call
>  >>>>>>>> PerlProc_setjmp()/PerlProc_longjmp(), which are typically
>  >>>>>>>> setjmp()/longjmp(), or maybe sigsetjmp()/siglongjmp() if you have
>  >>>>>>>> them.
>  >>>>>>>>
>  >>>>>>>> I think you're right that we should probably check that
> interp->refcnt
>  >>>>>>>>>
>  >>>>>>>>>
>  >>>>>>>>> 1 if we go ahead and pass interp into modperl_croak(). There
> aren't
>  >>>>>>>>
>  >>>>>>>>
>  >>>>>>>> too many calls, so this may be workable; we also have a few call
>  >>>>>>>> MP_RUN_CROAK()/MP_RUN_CROAK_RESET() calls to look at too. What
> worries
>  >>>>>>>> me is the (much larger number of!) calls to Perl_croak(). They
> will
>  >>>>>>>> also not return, so we presumably need to do cleanup before
> each one
>  >>>>>>>> of those too? Maybe we need a little wrapper function/macro to do
>  >>>>>>>> clean up and then call Perl_croak() and use that everywhere
> instead of
>  >>>>>>>> Perl_croak() (including the call inside modperl_croak(), of
> course)?
>  >>>>>>>
>  >>>>>>>
>  >>>>>>>
>  >>>>>>> The other approach I mentioned earlier was to try to do the
> cleanup in
>  >>>>>>> the eval() where the die() has landed. If that's possible then it
>  >>>>>>> might be a cleaner approach.
>  >>>>>>>
>  >>>>>>> In this case, I think we're inside the eval_pv done in
>  >>>>>>> modperl_filter_resolve_init_handler(). I only see three other
> eval*()s
>  >>>>>>> (one more eval_pv() and two eval_sv()s) around the mod_perl C
> source
>  >>>>>>> code so this could be worth pursuing.
>  >>>>>>
>  >>>>>>
>  >>>>>>
>  >>>>>> Hm, maybe stupid idea, but could we store the refcnt before the
> eval and
>  >>>>>> reset it after the eval? I presume that perlinterp is valid for the
>  >>>>>> evaluated code only when we are in the eval(), so if something
> increases
>  >>>>>> the refcnt in the eval() and forgets the decrease it later, we could
>  >>>>>> just decrease it ourselves using the set-eval-reset aproach.
>  >>>>>
>  >>>>>
>  >>>>>
>  >>>>> Not sure if we will be able to get modperl_interp_t * in these
>  >>>>> functions...
>  >>>>
>  >>>>
>  >>>>
>  >>>> I found a way how to get modperl_interp_t *, but I don't think we will
>  >>>> be able to cleanup after eval_*/call_*. We don't know what the right
>  >>>> refcount should be. I will use the Perl_croak wrapper approach
> instead.
>  >>>
>  >>>
>  >>>
>  >>> OK, this works as expected, but there's different problem...
>  >>>
>  >>> The problem is that when PerlInterpScope is set to connection, the
> refcnt is
>  >>> increased because of connection->pool cleanup method
>  >>> modperl_interp_pool_cleanup.
>  >>>
>  >>> The refcnt is decreased once the connection->pool is cleaned-up, but if
>  >>> there are more requests being handled by the connection, the refcnt is
>  >>> connection->pool cleanup is not called after the first request and
> therefore
>  >>> the second request has to take another interpreter. This lead to
> deadlock
>  >>> later with more requests coming.
>  >>>
>  >>> What's the point of PerlInterpScope and how it should work ideally?
>  >>
>  >>
>  >> I'm sorry, I don't know the answer to this. I've never made use of
>  >> this directive myself, so all I know is what I can read in the help:
>  >>
>  >>
> http://perl.apache.org/docs/2.0/user/config/config.html#C_PerlInterpScope_
>  >
>  >
>  > Could we drop PerlInterpScope? It does not work as expected and I
> think it cannot be fixed easily.
>  >
>  > In case of "PerlInterPScope connection", you can simply deadlock the
> mod_perl when sending more small requests using the same connection,
> because every request takes another interpreter and interpreter are not
> put back untill the connection is dispatched.
>  >
>  > There is no easy way how to solve that.
>  >
>  > I think the original idea behind PerlInterpScope has been to make the
> performance better when the single interpreter has been allocated and
> unallocated often during single connection, but with the pipelining,
> this is not easy to achieve with current mod_perl code.
>  >
>  > I would vote for removing the PerlInterpScope when building against
> httpd-2.4.x at least.
>
> If we can't easily fix it then I would certainly rather drop it (for now
> at least) than hold up 2.0.9 any longer.
>
> We can put a note in the Changes file about it to alert anyone that's
> currently using it, and see what feedback we get before investing our
> limited resources into trying to fix it.

OK, I should have time tomorrow for mod_perl, so I will commit my fix 
for that threaded mpm freeze and removed PerlInterpScope.

Jan Kaluza

> Steve
>
>  >
>  > Regards,
>  > Jan Kaluza
>  >
>  >
>  >> I recall it coming up before regardng another refcnt-related patch.
>  >> There is some talk about it in the messages of this thread:
>  >>
>  >> http://marc.info/?t=140191218700004&r=1&w=2
>  >>
>  >>
>  >>
>  >>>
>  >>> Regards,
>  >>> Jan Kaluza
>  >>>
>  >>>
>  >>>> Jan Kaluza
>  >>>>
>  >>>>>> Otherwise we would need a way to find out that:
>  >>>>>>
>  >>>>>> a) Perl_croak has been executed - could be easy, I bet this is
> possible
>  >>>>>> somehow with Perl API.
>  >>>>>> b) There is actually lost reference - Not sure how to do that,
> we don't
>  >>>>>> know if PUTBACK has been called before the Perl_croak or not...
>  >>>>>>
>  >>>>>> Regards,
>  >>>>>> Jan Kaluza
>  >>>>>>
>  >>>>>>
>  >>>>>>
> ---------------------------------------------------------------------
>  >>>>>> To unsubscribe, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>>>> For additional commands, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>>>>
>  >>>>>
>  >>>>>
>  >>>>> ---------------------------------------------------------------------
>  >>>>> To unsubscribe, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>>> For additional commands, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>>>
>  >>>>
>  >>>>
>  >>>> ---------------------------------------------------------------------
>  >>>> To unsubscribe, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>> For additional commands, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>>
>  >>>
>  >>>
>  >>> ---------------------------------------------------------------------
>  >>> To unsubscribe, e-mail: [email protected]
> <mailto:[email protected]>
>  >>> For additional commands, e-mail: [email protected]
> <mailto:[email protected]>
>  >>>
>  >
>