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

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]> wrote:
>>> On 15 May 2015 at 07:14, Jan Kaluža <[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]> 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]> 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...

> 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]
> For additional commands, e-mail: [email protected]
>