Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
Roy T. Fielding wrote: > > So, I'm -0.9 on the patch, even though it isn't much worse than > the existing code. I'd be -1 if I could be sure what it did. And I wouldn't debate it... back to the drawing board. As an improvement to the behavior I support it, in terms of style it got a -0.5 from me, but I didn't have the cycles to clean it up. Thanks for the kick in the ass to do so. It's time for the less-than-half-assed solution to WaitForMultipleObjects. Bill
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
On Dec 20, 2006, at 5:14 PM, William A. Rowe, Jr. wrote: * mpm_winnt: Fix return values from wait_for_many_objects. http://svn.apache.org/viewvc?view=rev&revision=428029 2.2.x version of patch: Trunk version works +http://people.apache.org/~wrowe/mpm_winnt_waits.patch +is easier to read (-U8) Well, I hate to be a stick in the mud, but that code sure seems like a severely sucking hack. Is WaitForMultipleObjects() a severely broken API that requires knowledge of several constant values and their relative offsets? Or are these private constants that lack documentation? Why perform a spin loop that checks while((time(NULL) < tStopTime) && (dwRet == WAIT_FAILED)); the former condition being expensive while the latter is cheap? It is actually testing WAIT_TIMEOUT (looping until we don't get a timeout), but that is really hard to see now because of the hackery on dwRet values. And why are there three different exit points out of the loop? And WTF doesn't it just increment dwIndex += MAXIMUM_WAIT_OBJECTS? And what is the return value supposed to mean to callers? And why is it different from the version on trunk? Sleep(1000)? Yowza. So, I'm -0.9 on the patch, even though it isn't much worse than the existing code. I'd be -1 if I could be sure what it did. Roy
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
Jim Jagielski wrote: > William A. Rowe, Jr. wrote: >>> The other fatal bug is: >>> * mpm_winnt: Fix return values from wait_for_many_objects. http://svn.apache.org/viewvc?view=rev&revision=428029 2.2.x version of patch: Trunk version works +http://people.apache.org/~wrowe/mpm_winnt_waits.patch +is easier to read (-U8) +1: mturk, wrowe >>> Which i've created another, easier-to-read example of the backport, if you >>> didn't follow the change last time please read the new patch and chime in >>> a third +1? You'll note it's simply a matter of unsetting errno before we >>> invoke the wait, so we know if the 64th, 128th, etc are real errors or in >>> fact indexes into the wait list array. >>> >>> That's it. I'd like to roll tonight so the vote can be closed by the 22nd. >> I'd like to have was dependent on someone tossing in a vote here /shrug. > > I could vote +1 by inspection, but I can't test... Of course... had I expected anyone would test, I wouldn't have wasted the time and effort to generate a legigble diff -U8 - which by the way is somewhat tricky... svn diff --diff-cmd=diff --extentions=-U8 are both required since the built-in-diff'er doesn't know -u8 or -U8, and the diff-cmd is an app path/name, not a command line. Guess it was worth wasting the time to learn this even if this exercise doesn't have any net results.
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
William A. Rowe, Jr. wrote: > > > The other fatal bug is: > > > >> * mpm_winnt: Fix return values from wait_for_many_objects. > >> http://svn.apache.org/viewvc?view=rev&revision=428029 > >>2.2.x version of patch: > >> Trunk version works > >> +http://people.apache.org/~wrowe/mpm_winnt_waits.patch > >> +is easier to read (-U8) > >>+1: mturk, wrowe > > > > Which i've created another, easier-to-read example of the backport, if you > > didn't follow the change last time please read the new patch and chime in > > a third +1? You'll note it's simply a matter of unsetting errno before we > > invoke the wait, so we know if the 64th, 128th, etc are real errors or in > > fact indexes into the wait list array. > > > > That's it. I'd like to roll tonight so the vote can be closed by the 22nd. > > I'd like to have was dependent on someone tossing in a vote here /shrug. > I could vote +1 by inspection, but I can't test... -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ "If you can dodge a wrench, you can dodge a ball."
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
William A. Rowe, Jr. wrote: > next time http://issues.apache.org/bugzilla/show_bug.cgi?id=37680 > would help so the fruit hangs even lower. Thanks Jim! > The other fatal bug is: > >> * mpm_winnt: Fix return values from wait_for_many_objects. >> http://svn.apache.org/viewvc?view=rev&revision=428029 >>2.2.x version of patch: >> Trunk version works >> +http://people.apache.org/~wrowe/mpm_winnt_waits.patch >> +is easier to read (-U8) >>+1: mturk, wrowe > > Which i've created another, easier-to-read example of the backport, if you > didn't follow the change last time please read the new patch and chime in > a third +1? You'll note it's simply a matter of unsetting errno before we > invoke the wait, so we know if the 64th, 128th, etc are real errors or in > fact indexes into the wait list array. > > That's it. I'd like to roll tonight so the vote can be closed by the 22nd. I'd like to have was dependent on someone tossing in a vote here /shrug.
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
If we're in backport season, I have a quick patch that I whipped up
yesterday to fix mod_disk_cache on 2.2.x on systems that have
APR_SENDFILE_ENABLED but EnableSendfile Off.
Issac
William A. Rowe, Jr. wrote:
> [EMAIL PROTECTED] wrote:
>> Votes and Notes; the first four I would like to see applied before
>> tagging 1.2.4; add a better patch for the winnt mpm wait patch
>> that makes this reviewable (sometimes -U3 just isn't enough context.)
>
> As Ruediger guessed and committed (thanks!) we are now two more patches down.
>
> htcacheclean and httxt2dbm was a considerable flaw (considered that they
> didn't behave as documented, and these utilities are supposed to be easily
> moved around from box to box or run from a remote machine) and I'm about
> to commit that.
>
> I see two final state bugs... I'm about to finish vetting niq's patch for
> listen reconfig which pegs httpd at 100% cpu
>
>* core: PR#37680
> Fix NONBLOCK status of listening sockets on restart/graceful
> http://svn.apache.org/viewvc?view=rev&revision=487901
> +1: niq
>
> someone please add a third voice on Nick's patch? I'm guessing nobody
> bothered, next time http://issues.apache.org/bugzilla/show_bug.cgi?id=37680
> would help so the fruit hangs even lower.
>
> The other fatal bug is:
>
>> * mpm_winnt: Fix return values from wait_for_many_objects.
>> http://svn.apache.org/viewvc?view=rev&revision=428029
>>2.2.x version of patch:
>> Trunk version works
>> +http://people.apache.org/~wrowe/mpm_winnt_waits.patch
>> +is easier to read (-U8)
>>+1: mturk, wrowe
>
> Which i've created another, easier-to-read example of the backport, if you
> didn't follow the change last time please read the new patch and chime in
> a third +1? You'll note it's simply a matter of unsetting errno before we
> invoke the wait, so we know if the 64th, 128th, etc are real errors or in
> fact indexes into the wait list array.
>
> That's it. I'd like to roll tonight so the vote can be closed by the 22nd.
> If another patch today fixes a runaway cpu or platform fault I might fix,
> if it's behavioral/optimization I won't be holding for it. Those can go in
> the next 2 - 6 weeks whenever Jim rolls the release he's been aspiring to.
Index: modules/cache/mod_disk_cache.c
===
--- modules/cache/mod_disk_cache.c (revision 488297)
+++ modules/cache/mod_disk_cache.c (working copy)
@@ -359,6 +359,10 @@
cache_info *info;
disk_cache_object_t *dobj;
int flags;
+#if APR_HAS_SENDFILE
+core_dir_config *pdconf = ap_get_module_config(r->per_dir_config,
+ &core_module);
+#endif
h->cache_obj = NULL;
@@ -453,7 +457,8 @@
/* Open the data file */
flags = APR_READ|APR_BINARY;
#ifdef APR_SENDFILE_ENABLED
-flags |= APR_SENDFILE_ENABLED;
+flags |= ((pdconf->enable_sendfile == ENABLE_SENDFILE_OFF)
+ ? 0 : APR_SENDFILE_ENABLED);
#endif
rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
Nick Kew wrote: > On Tue, 19 Dec 2006 16:34:24 -0600 > "William A. Rowe, Jr." <[EMAIL PROTECTED]> wrote: > >> I see two final state bugs... I'm about to finish vetting niq's patch >> for listen reconfig which pegs httpd at 100% cpu > > For the record, not my patch. It's attached to the PR, and the > contributor is currently acknowledged in /trunk/CHANGES After I clicked send, I realized that :)
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
On Tue, 19 Dec 2006 16:34:24 -0600 "William A. Rowe, Jr." <[EMAIL PROTECTED]> wrote: > I see two final state bugs... I'm about to finish vetting niq's patch > for listen reconfig which pegs httpd at 100% cpu For the record, not my patch. It's attached to the PR, and the contributor is currently acknowledged in /trunk/CHANGES -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Re: svn commit: r488780 - /httpd/httpd/branches/2.2.x/STATUS
[EMAIL PROTECTED] wrote: > Votes and Notes; the first four I would like to see applied before > tagging 1.2.4; add a better patch for the winnt mpm wait patch > that makes this reviewable (sometimes -U3 just isn't enough context.) As Ruediger guessed and committed (thanks!) we are now two more patches down. htcacheclean and httxt2dbm was a considerable flaw (considered that they didn't behave as documented, and these utilities are supposed to be easily moved around from box to box or run from a remote machine) and I'm about to commit that. I see two final state bugs... I'm about to finish vetting niq's patch for listen reconfig which pegs httpd at 100% cpu * core: PR#37680 Fix NONBLOCK status of listening sockets on restart/graceful http://svn.apache.org/viewvc?view=rev&revision=487901 +1: niq someone please add a third voice on Nick's patch? I'm guessing nobody bothered, next time http://issues.apache.org/bugzilla/show_bug.cgi?id=37680 would help so the fruit hangs even lower. The other fatal bug is: > * mpm_winnt: Fix return values from wait_for_many_objects. > http://svn.apache.org/viewvc?view=rev&revision=428029 >2.2.x version of patch: > Trunk version works > +http://people.apache.org/~wrowe/mpm_winnt_waits.patch > +is easier to read (-U8) >+1: mturk, wrowe Which i've created another, easier-to-read example of the backport, if you didn't follow the change last time please read the new patch and chime in a third +1? You'll note it's simply a matter of unsetting errno before we invoke the wait, so we know if the 64th, 128th, etc are real errors or in fact indexes into the wait list array. That's it. I'd like to roll tonight so the vote can be closed by the 22nd. If another patch today fixes a runaway cpu or platform fault I might fix, if it's behavioral/optimization I won't be holding for it. Those can go in the next 2 - 6 weeks whenever Jim rolls the release he's been aspiring to.
