Re: Comments on non-unique naming convention for closures

hi Terry, Please share the new PHPT tests or better attach them to bug report. (D) looks reasonable simple. I thinks it must be a good enough solution. For ZTS we can use thread_id in addition to PID (or instead of it). Also we may use a simple thread specific counter instead of microtime - CG(key_counter). Thanks. Dmitry. On Thu, Dec 5, 2013 at 9:17 PM, Terry Ellison <ellison.terry@gmail.com>wrote: > 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?" ... > > >