Return-Path: Delivered-To: apmail-perl-dev-archive@www.apache.org Received: (qmail 6611 invoked from network); 3 May 2007 11:33:20 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 3 May 2007 11:33:20 -0000 Received: (qmail 89255 invoked by uid 500); 3 May 2007 11:33:26 -0000 Delivered-To: apmail-perl-dev-archive@perl.apache.org Received: (qmail 89238 invoked by uid 500); 3 May 2007 11:33:26 -0000 Mailing-List: contact dev-help@perl.apache.org; run by ezmlm Precedence: bulk list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@perl.apache.org Received: (qmail 89227 invoked by uid 99); 3 May 2007 11:33:25 -0000 Received: from herse.apache.org (HELO herse.apache.org) (140.211.11.133) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 03 May 2007 04:33:25 -0700 X-ASF-Spam-Status: No, hits=-0.0 required=10.0 tests=SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (herse.apache.org: domain of torsten.foertsch@gmx.net designates 213.165.64.20 as permitted sender) Received: from [213.165.64.20] (HELO mail.gmx.net) (213.165.64.20) by apache.org (qpsmtpd/0.29) with SMTP; Thu, 03 May 2007 04:33:18 -0700 Received: (qmail invoked by alias); 03 May 2007 11:32:56 -0000 Received: from dialin096079.justdsl.de (EHLO opi.home) [85.25.96.79] by mail.gmx.net (mp015) with SMTP; 03 May 2007 13:32:56 +0200 X-Authenticated: #1700068 X-Provags-ID: V01U2FsdGVkX1/Tt6chZznTl6Su0caxQuZH2k6+A56fzlhBwcqenu gkiyqm6qHC3d5Z Received: by opi.home (Postfix, from userid 1000) id 7B8D8C2D14; Thu, 3 May 2007 13:32:51 +0200 (CEST) From: Torsten Foertsch To: dev@perl.apache.org Subject: [PATCH]Re: threaded server + scope=handler: modperl_response_handler vs. modperl_response_handler_cgi Date: Thu, 3 May 2007 13:32:45 +0200 User-Agent: KMail/1.9.5 Cc: "Philippe M. Chiasson" References: <200704301208.28418.torsten.foertsch@gmx.net> In-Reply-To: <200704301208.28418.torsten.foertsch@gmx.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart14731647.Y7PyzEs42o"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <200705031332.50712.torsten.foertsch@gmx.net> X-Y-GMX-Trusted: 0 X-Virus-Checked: Checked by ClamAV on apache.org --nextPart14731647.Y7PyzEs42o Content-Type: multipart/mixed; boundary="Boundary-01=_ehcOG08wW9Br/tB" Content-Transfer-Encoding: 7bit Content-Disposition: inline --Boundary-01=_ehcOG08wW9Br/tB Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Monday 30 April 2007 12:08, Torsten Foertsch wrote: > This means in both cases modperl_interp_select is called twice but for the > perl-script case rcfg->interp =3D interp is set. > > modperl_interp_select first looks if rcfg->interp is set and returns it. > Only if it's not it looks at other places for an interpreter. > > This means a perl-script handler needs 1 interpreter to handle the respon= se > phase while a modperl handler needs 2. On Tuesday 01 May 2007 12:24, Torsten Foertsch wrote: > On Monday 30 April 2007 22:22, Philippe M. Chiasson wrote: > > And that doesn't seem logical at all. In all cases, since the initial > > int=3D erpreter > > is tied up in modperl_response_handler() a new one shouldn't be needed > > to=3D dispatch > > that handler part. > > I have found similar bug. Try > > $r->push_handlers(PerlCleanupHandler=3D>'some_named_function') > > from a TranslationHandler or so. [...] > Or maybe a cleaner way is to use interp->refcnt. Simply increment it each > time modperl_interp_select returns it. For what I can see refcnt isn't us= ed > at all by now. Yes, on a second thought that is what I'd prefer. The attached patch fixes these 2 issues. It is against r534739. I have test= ed=20 it with prefork and worker with a httpd 2.2.3 on linux. What does it change? 1) it moves the handling of rcfg->interp from modperl_response_handler and= =20 modperl_response_handler_cgi to modperl_interp_select /=20 modperl_interp_unselect. 2) If interp is saved to rcfg it counts as a reference to it. So refcnt is= =20 incremented. All other changes are MP_TRACE macros to see what is going on. Now the program flow is this: If scope=3D=3Drequest, connection or subrequest the PUTBACK flag is allways= off.=20 Interp is stored in the pool and modperl_interp_unselect is called by pool= =20 cleanup. Nothing has changed. If scope=3D=3Dhandler PUTBACK is on. Interp is not stored in a pool but all= ways in=20 rcfg. So recursive calls to modperl_interp_select will find it there and=20 increment its refcnt. Hence an equal number of calls to=20 modperl_interp_unselect is required to really release the interp. So, both modperl_response_handler allocate an interpreter then call=20 modperl_callback_run_handlers which recursively allocates the same interp=20 found in rcfg. The putback operation in modperl_callback_run_handlers then= =20 only decrements the refcnt and the putback in modperl_response_handler real= ly=20 does the unselect. But unselect has to call PerlCleanupHandlers. So=20 modperl_interp_select is called again (while in modperl_interp_unselect),=20 finds the interp in rcfg and increments its refcnt. After all cleanup=20 handlers are run modperl_callback_run_handlers calls unselect which simply= =20 decrements the refcnt. Then modperl_config_request_cleanup returns to the=20 outer modperl_interp_unselect and the interp is really freed. Cleanup handlers installed via PerlCleanupHandler are thus called each time= an=20 interp is really freed. $r->pool cleanups behave different. A pool cleanup_register also increments= an=20 interp's refcnt. Now if a pool cleanup is installed say in the translation= =20 phase the interp has allways a refcnt>0. So it is not released until the po= ol=20 is destroyed. And since interp is stored in rcfg it is sticky to the reques= t.=20 All other phases after the pool cleanup installation will be handled by the= =20 same interp. Torsten --Boundary-01=_ehcOG08wW9Br/tB Content-Type: text/x-diff; charset="iso-8859-15"; name="scope==handler.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="scope==handler.patch" =2D-- trunk/src/modules/perl/mod_perl.c 2007-05-03 09:35:12.000000000 +0200 +++ working-copy/src/modules/perl/mod_perl.c 2007-05-03 10:23:54.000000000 = +0200 @@ -1027,6 +1027,7 @@ } =20 #ifdef USE_ITHREADS + MP_TRACE_i(MP_FUNC, "before interp_select\n"); interp =3D modperl_interp_select(r, r->connection, r->server); aTHX =3D interp->perl; #endif @@ -1039,6 +1040,7 @@ retval =3D modperl_response_handler_run(r, TRUE); =20 #ifdef USE_ITHREADS + MP_TRACE_i(MP_FUNC, "before PUTBACK\n"); if (MpInterpPUTBACK(interp)) { /* PerlInterpScope handler */ modperl_interp_unselect(interp); @@ -1064,11 +1066,9 @@ } =20 #ifdef USE_ITHREADS + MP_TRACE_i(MP_FUNC, "before interp_select\n"); interp =3D modperl_interp_select(r, r->connection, r->server); aTHX =3D interp->perl; =2D if (MpInterpPUTBACK(interp)) { =2D rcfg->interp =3D interp; =2D } #endif =20 modperl_perl_global_request_save(aTHX_ r); @@ -1105,7 +1105,6 @@ if (MpInterpPUTBACK(interp)) { /* PerlInterpScope handler */ modperl_interp_unselect(interp); =2D rcfg->interp =3D NULL; } #endif =20 =2D-- trunk/src/modules/perl/modperl_interp.c 2007-05-03 09:35:12.000000000= +0200 +++ working-copy/src/modules/perl/modperl_interp.c 2007-05-03 10:32:28.0000= 00000 +0200 @@ -273,9 +273,12 @@ modperl_interp_t *interp =3D (modperl_interp_t *)data; modperl_interp_pool_t *mip =3D interp->mip; =20 + MP_TRACE_i(MP_FUNC, "interp=3D0x%lx, refcnt=3D%d\n", + (unsigned long)interp, interp->refcnt); + if (interp->refcnt !=3D 0) { --interp->refcnt; =2D MP_TRACE_i(MP_FUNC, "interp=3D0x%lx, refcnt=3D%d\n", + MP_TRACE_i(MP_FUNC, "interp=3D0x%lx, refcnt is now %d, unselect do= ne\n", (unsigned long)interp, interp->refcnt); return APR_SUCCESS; } @@ -286,6 +289,7 @@ MP_dRCFG; modperl_config_request_cleanup(interp->perl, r); MpReqCLEANUP_REGISTERED_Off(rcfg); + rcfg->interp =3D NULL; } =20 MpInterpIN_USE_Off(interp); @@ -295,6 +299,9 @@ =20 modperl_tipool_putback_data(mip->tipool, data, interp->num_requests); =20 + MP_TRACE_i(MP_FUNC, "interp=3D0x%lx released, unselect done\n", + (unsigned long)interp); + return APR_SUCCESS; } =20 @@ -400,9 +407,10 @@ * e.g. modperl_response_handler_cgi(), that interpreter will * be here */ + rcfg->interp->refcnt++; MP_TRACE_i(MP_FUNC, =2D "found interp 0x%lx in request config\n", =2D (unsigned long)rcfg->interp); + "found interp 0x%lx in request config, refcnt is now %d= \n", + (unsigned long)rcfg->interp, rcfg->interp->refcnt); return rcfg->interp; } =20 @@ -486,8 +494,10 @@ interp->request =3D r; MpReqCLEANUP_REGISTERED_On(rcfg); MpInterpPUTBACK_On(interp); + rcfg->interp =3D interp; } else { + interp->request =3D NULL; /* avoid early cleanup see modperl_inter= p_unselect */ if (!p) { /* should never happen */ MP_TRACE_i(MP_FUNC, "no pool\n"); =2D-- trunk/src/modules/perl/modperl_interp.h 2007-05-03 09:35:12.000000000= +0200 +++ working-copy/src/modules/perl/modperl_interp.h 2007-05-03 10:33:50.0000= 00000 +0200 @@ -89,10 +89,12 @@ #define MP_dINTERP_SELECT(r, c, s) \ pTHX; \ modperl_interp_t *interp =3D NULL; \ + MP_TRACE_i(MP_FUNC, "before interp_select\n"); \ interp =3D modperl_interp_select(r, c, s); \ aTHX =3D interp->perl =20 #define MP_INTERP_PUTBACK(interp) \ + MP_TRACE_i(MP_FUNC, "before PUTBACK\n"); \ if (interp && MpInterpPUTBACK(interp)) { \ modperl_interp_unselect(interp); \ } =2D-- trunk/src/modules/perl/modperl_callback.c 2007-05-03 09:35:12.0000000= 00 +0200 +++ working-copy/src/modules/perl/modperl_callback.c 2007-05-03 10:25:02.00= 0000000 +0200 @@ -189,6 +189,7 @@ c =3D r->connection; } if (r || c) { + MP_TRACE_i(MP_FUNC, "before interp_select\n"); interp =3D modperl_interp_select(r, c, s); aTHX =3D interp->perl; } --Boundary-01=_ehcOG08wW9Br/tB-- --nextPart14731647.Y7PyzEs42o Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (GNU/Linux) iD8DBQBGOchiwicyCTir8T4RAiLgAKCzqgtNzY6johsedAvkPm55jKlgUwCgwYbV s21p3CZrOvJ9xw9t4xcWnNE= =untq -----END PGP SIGNATURE----- --nextPart14731647.Y7PyzEs42o--