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

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]> 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...

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.

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