On 28 August 2013 08:14, Michael Wallner <m...@php.net> wrote: > Hi Gustavo, thank you for your review! > > On 27 August 2013 23:17, Gustavo Lopes <glo...@nebm.ist.utl.pt> wrote:
>> I think it's generally a good idea, but I have some concerns: >> >> * The whole point of PG(enable_post_data_reading) is to be able to read the >> input of the fly. If I read your patch correctly, the data is completely >> gobbled when you open php://input and then the temp file is reminded. This >> will result in a serious performance degradation on large inputs, as >> processing can only start after everything has been read. The behavior >> should be different if PG(enable_post_data_reading) is false. > > Ok, *I* thought the whole point of enable_post_data_reading is not to > swamp your memory :) We can have that behaviour of course, but then > the input stream is not reusable. ... >> * The php://input streams simply forward the calls to >> SG(request_info).request_body, so if you open two, the per-stream cursor >> position may not match the position of the wrapped stream (even if it >> matched, we wouldn't want one stream to affect the position of the other). I > > True! So my input stream handles concurrency as bad as the current > implementation... 8) > >> don't see any easy way out here; all I can think of is is duping the file >> descriptor for the temporary file in these situations. But then the book >> keeping for php://input would be more complicated. > > I think that shouldn't be too hard to fix. But it depends on concern no.1 :) Fixed. The input stream is reusable *and* may be used JIT. https://github.com/m6w6/php-src/compare/slim-postdata-merge -- Regards, Mike -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
