Hi Anatol!
Anatol Belski wrote:
>> -----Original Message-----
>> From: Christoph Becker [mailto:cmbecke...@gmx.de]
>> Sent: Wednesday, July 1, 2015 12:55 AM
>> To: Anatol Belski; 'PHP Internals'
>> Cc: 'Pierre Joye'; 'Kalle Sommer Nielsen'; 'Dmitry Stogov'; 'Xinchen Hui';
>> 'Nikita
>> Popov'
>> Subject: [PHP-DEV] Re: Fixes for #69900 and #69963
>>
>> It appears to be possible to set the buffer size by passing a nSize argument
>> > 0 to
>> CreatePipe[1][2]. Maybe it's reasonable to use 16 or 64 kB by default?
>
> Does it work? To my current experience, the buffer size is not to change
> manually, the system pipe manager decides it. So even it could work on one
> machine, it could fail on another. Still one could verify with
> GetNamedPipeInfo, which didn't show reliable results for me. In general, a
> bigger pipe buffer size is what makes the Linux implementation less
> vulnerable. And, because most of the previous behavior, blocking reads are
> expected for some long running daemons and co. But here's the terrible mix
> about "read forever" and "dead lock", I don't see a solution as long as we
> use anonymous pipes on Windows, but this is completely another story to
> discuss. Thus - the addition of user space args, count with users who know
> what they're doing, the >99% of usage is covered by what we have now :)
I did only a quick check on a single machine, and that worked, but of
course that doesn't mean much. If it's generally a problem, it seems to
be best to stick with the default, and to document the limited buffer size.
>> I did some quick tests and the patch works fine. However,
>> proc_open_bug69900.phpt fails on my machine (Win 7), because 0ms are
>> reported for all but the first read (microtime issue on older Windows').
>> Maybe it's better to EXPECT something like "took less than 1 ms"?
>
> Good catch. Or maybe 0%sms ... or alike ... still a platform issue, nothing
> with PHP itself. The speaking result here is that a persistent connection
> with pushing small chunks delivers correct, maybe removing timings would
> suffice as well, please advise.
When I run the test without the patch it works, except for the timings.
So it seems they are necessary for the test to be useful. Even though
such timings are generally fragile, I don't have a better idea.
However, the "0%sms" test[1] fails here. It seems that %s doesn't match
an empty string. I've attached a patch that works for me (I hope it
gets through to the list). Please consider to use it.
>> I have noticed that you've changed the bitfields of struct
>> php_stdio_stream_data (in plain_wrapper.c). Wouldn't it be better to do:
>>
>> - unsigned is_pipe_blocking;
>> + unsigned is_pipe_blocking:1;
>> + unsigned _reserved:28;
>
> Nope, reserved was there to indicate the alignment (between 32- and 64-bit?).
> Replacing _reserved does no change to the struct size. Still one could
> preserve it on non Windows, but it's kinda useless as any further change
> would need an additional field, so then the order were the question. I was
> thinking about making it zend_bool, still what other three zend_bools would
> go into the remaining space? Still could be done if some situation arises, so
> no loss here - it's encapsulated in a C file, not exposed in a header.
Ah, yes, I see. On 32bit architectures it seems to increase the struct
size from 64 to 68 bytes (checked with MSVC), though. I consider this
an extremely minor issue, but still it bugs me a bit to have three
bitfields of size 1 and another boolean declared as unsigned. Maybe
it's cleaner to declare all of is_process_pipe, is_pipe, cached_fstat
and is_pipe_blocking as zend_bool?
[1]
<https://github.com/php/php-src/commit/7fbfd81d3e0242d73f7662ecb830c857704a4c5e>
--
Christoph M. Becker
ext/standard/tests/streams/proc_open_bug69900.phpt | 24 +++++++++++-----------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/ext/standard/tests/streams/proc_open_bug69900.phpt
b/ext/standard/tests/streams/proc_open_bug69900.phpt
index b352329..c98fd97 100644
--- a/ext/standard/tests/streams/proc_open_bug69900.phpt
+++ b/ext/standard/tests/streams/proc_open_bug69900.phpt
@@ -33,7 +33,7 @@ for($i = 0; $i < 10; $i++){
$t1 = microtime(1);
echo $s;
- echo "fgets() took ", (($t1 - $t0)*1000), "ms\n";
+ echo "fgets() took ", (($t1 - $t0)*1000 > 1 ? 'more' : 'less'), " than
1 ms\n";
}
fclose($pipes[0]);
@@ -48,25 +48,25 @@ proc_close($process);
$fl = dirname(__FILE__) . DIRECTORY_SEPARATOR . "test69900.php";
@unlink($fl);
?>
---EXPECTF--
+--EXPECT--
hello0
-fgets() took %d%sms
+fgets() took more than 1 ms
hello1
-fgets() took 0%sms
+fgets() took less than 1 ms
hello2
-fgets() took 0%sms
+fgets() took less than 1 ms
hello3
-fgets() took 0%sms
+fgets() took less than 1 ms
hello4
-fgets() took 0%sms
+fgets() took less than 1 ms
hello5
-fgets() took 0%sms
+fgets() took less than 1 ms
hello6
-fgets() took 0%sms
+fgets() took less than 1 ms
hello7
-fgets() took 0%sms
+fgets() took less than 1 ms
hello8
-fgets() took 0%sms
+fgets() took less than 1 ms
hello9
-fgets() took 0%sms
+fgets() took less than 1 ms
===DONE===
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
