[Apologies for all the top-posting replies, btw. Blame Outlook! I have a macro somewhere to fix it, but haven't installed it yet on my new machine...]
I've committed the ModPerl::MM fix, renaming $b to $build, and generally borrowing the style of ModPerl::BuildMM (although it's all rather ugly anyway). I will try to bisect to find where the OOM crash has crept in. And I definitely agree that any code commented "should not happen" ought to assert when it does happen so that we (developers) can catch it in debug builds. To that end, the MP_ASSERT macro that you've added to the threading branch would be useful to have in trunk ASAP. -----Original Message----- From: Torsten Förtsch [mailto:torsten.foert...@gmx.net] Sent: 14 February 2012 13:19 To: Steve Hay Cc: dev@perl.apache.org; Fred Moyer Subject: Re: [RELEASE CANDIDATE]: mod_perl-2.0.6 RC1 On Tuesday, 14 February 2012 09:58:51 Steve Hay wrote: > The patch below fixes the problems with MP_LIBNAME (and cwd) not being > set correctly when building Apache::Reload and Apache::SizeLimit. > > The problem was that the file-level $b variable was created only once > when ModPerl::MM was first loaded, but that happened before > Apache2::BuildConfig.pm was written. The patch simply rewrites $b > (instead of writing a lexical $build which was never used!) later on, > by which time Apache2::BuildConfig.pm has been created. > > If the patch looks ok then shall I apply it before RC2 is rolled? > > (Needless to say, this doesn't fix the more serious problem of the > test suite still failing when httpd.exe crashes with an out of memory > error part-way through...) > > Index: lib/ModPerl/MM.pm > =================================================================== > --- lib/ModPerl/MM.pm (revision 1240026) > +++ lib/ModPerl/MM.pm (working copy) > @@ -128,7 +128,7 @@ > $eu_mm_mv_all_methods_overriden++; > } > > - my $build = build_config(); > + $b = build_config(); > my_import(__PACKAGE__); > > # set top-level WriteMakefile's values if weren't set already Apart from the fact that $b is a really nasty name for a global variable I don't mind. But that's not your fault. Be it $x or $y or even $B I wouldn't complain either. But $a and $b look like out of a sort {} block to me. Why don't you commit the patch and see what happens. The good thing with SVN is that we can always go back. On the OOM, is there a special commit where it starts? While working on the threading branch I discovered at least one occurrence (modperl_filter_f_cleanup) where an interpreter was used *after* it has been put back to the pool. That means at this point it might have already been assigned to another thread. Thanks, Steve, for reminding that Perl sometimes picks the interpreter by PERL_GET_CONTEXT. That was the key. See "svn log -c1243509" for more information. I don't know if the bug is also present in trunk but I strongly believe so. In principle this may cause arbitrary effects. The other occurrence mentioned in the commit message can happen only with "PerlInterpScope handler", I think. Another note, I have seen quite a few pieces of code reading: if (...) { /* should not happen */ return NULL; } This is really bad because it hides bugs. It does not fix anything. Wouldn't it be better to abort() here? Or ap_assert()? Torsten Förtsch -- Need professional modperl support? Hire me! (http://foertsch.name) Like fantasy? http://kabatinte.net --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org For additional commands, e-mail: dev-h...@perl.apache.org
