Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:68629
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain nebm.ist.utl.pt from 193.136.128.21 cause and error)
Message-ID: <521D174D.4030802@nebm.ist.utl.pt>
Date: Tue, 27 Aug 2013 23:17:01 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Thunderbird/22.0
MIME-Version: 1.0
To: internals@lists.php.net, mike@php.net
References: <CALJZF3iw7uzUN2z-aBfO34c=x9tY-gCFaDHRRZq-JYpxHgdmrg@mail.gmail.com>
In-Reply-To: <CALJZF3iw7uzUN2z-aBfO34c=x9tY-gCFaDHRRZq-JYpxHgdmrg@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] PROPOSAL: temp stream for post_data
From: glopes@nebm.ist.utl.pt (Gustavo Lopes)

On 27-08-2013 14:08, Michael Wallner wrote:
> Hi,
>
> I prepared a patch to replace sapi_globals' request_info post_data and
> raw_post_data with a temp stream and remove support for
> HTTP_RAW_POST_DATA. [1]
>
> PROS:
> * save up to 300% on post_data_len memory (on non-form POSTs)
> * a local siege (c=512/512, 2.4k form/2.2k json) showed no (negative)
> performance impact; see attached logs
> * reusable php://input stream
> * ...
>
> CONS:
> * no more $HTTP_RAW_POST_DATA, where BC could easily provided with a one-liner:
> $GLOBALS["HTTP_RAW_POST_DATA"]=file_get_contents("php://input");
> * memory is cheap
> * ???
>

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.

* I don't see SG(request_info).request_body being closed anywhere. Is 
this relying just on auto-cleanup? I recall that being problematic in 
debug mode unless you use php_stream_auto_cleanup().

* 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 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.

* The cast added php_stream_get_resource_id() seems unnecessary; may 
hide bugs. If you just want to be able to pass void* values, better to 
put an inline function there with a php_stream* parameter, that way you 
would have a compiler warning if you passed anything but a php_stream* 
or a void* (though this would break taking a pointer).


-- 
Gustavo Lopes