Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70501
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.180 as permitted sender)
Message-ID: <52A0B513.2010108@gmail.com>
Date: Thu, 05 Dec 2013 17:17:07 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.1
MIME-Version: 1.0
To: Dmitry Stogov <dmitry@zend.com>
CC: PHP Internals <internals@lists.php.net>
References: <528CE64A.1020303@gmail.com>
In-Reply-To: <528CE64A.1020303@gmail.com>
Content-Type: multipart/alternative;
 boundary="------------020400040308020701060102"
Subject: Re: Comments on non-unique naming convention for closures
From: ellison.terry@gmail.com (Terry Ellison)

--------------020400040308020701060102
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Dmitry,

The underlying issue here is the assumption that 
build_runtime_defined_function_key() in zend_compile.c generates unique 
keys.  I've realised that this impacts not only closures but also 
late-bound functions and classes.  I've now produced PHPTs which 
demonstrates the failure in all three cases.  Whilst hashing a closure 
source text has an acceptable compile-time overhead,  the case for 
classes (with their larger source lengths) is more of a concern.

I am now proposing an alternative hash based on:

A.  The class / function name (as per current)
B.  The reference source file (as per current)
C.  The address of the buffer (as per current)
D.  The PID and microtime at the request start.
E.  A running count of the function/classed compiled during the request.

(A-C) were assumed unique but, as #64291 and #65915 show, are not.

(D) should be sufficient to prevent race conditions across different 
OPcached threads / processes, but this has the advantage that it only 
needs to be computed once per request.

(E) guarantees uniqueness within thread.

The only weakness of D and E alone is a possible race on ZTS builds 
where two clashing threads start at the same microsecond, which (C) will 
split.  If this is considered too unwieldy then we could always drop B 
as C-E guarantee uniqueness, and B is never exposed to the programmer 
anyway (Xdebug and the error logger use the appropriate fields in the 
zend_function / zend_class record and not its key in the corresponding 
EG table.

This also has the advantage that the main change can now be localised to 
build_runtime_defined_function_key().

//Terry

> The following bugs relate to this discussion:
>
>     #64291 Indeterminate GC of evaled lambda function resources
>     #65915 Inconsistent results with require return value
>
> I don't want to discuss these or the specific fixes here, since I can 
> work up a fix and discuss them in the bugrep.  However,
> my one-sentence Q is "should we replace the naming convention for 
> closures with a truly unique one?"  ...


--------------020400040308020701060102--