Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecke...@gmx.de] > Sent: Thursday, July 2, 2015 1:32 AM > To: Anatol Belski; 'PHP Internals' > Cc: 'Pierre Joye'; 'Kalle Sommer Nielsen'; 'Dmitry Stogov'; 'Xinchen Hui'; > 'Nikita > Popov' > Subject: Re: [PHP-DEV] Re: Fixes for #69900 and #69963 > > 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. > Yeah, this is interesting anyway. As mentioned, I haven't seen very reliable results with it. Also reading once more the MSDN docs, they say " The size is only a suggestion; the system uses the value to calculate an appropriate buffering mechanism. If this parameter is zero, the system uses the default buffer size." ... which probably has the point :( For instance, if you set the buffer size to 1 byte (say want no buffering), that most likely won't work. It looks more like the buffering in the C runtime where we cannot do much.
But where you've got the point is that - on systems where it works, it could be one more small detail improving behavior (however would be hard to debug and reproduce these parts on different systems). Though bugs like #64438 were to recheck again. But where we directly use CreatePipe is proc_open. Anyway, it's needs more investigation. Another thing for proc_open which could be useful I thought about were another win only config to pass the timeout from user land. Then it'd combine both safe pipe operations and pseudo blocking read behavior. However this way the timeout will need to be passed to the streams, and php_stdio_stream_data struct will have be extended with at least one more 4 byte member. In general, IMHO, we probably need some refactoring there, instead of doing these hotfixes and adding evermore #ifdefs and all that :) Particularly with pipes, blocking, sockets, file descriptors, syscalls, etc. ... a stronger abstraction by platform would give more flexibility to do fixes (like not waiting for the next major), but what is more important - it would allow usage of platform specific APIs which often bring better results. The world gets evermore asynchronous, anyway :) > >> 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. Yeah, makes sense. I've applied this, but still not sure the timings should be kept. Whereby no other choice - it's the only way to check the correct behavior, but it'll fail with valgrind. > > >> 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? Applied your first suggestion, thanks! Kind regards Anatol -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
