Re: Comments on non-unique naming convention for closures

On 21/11/13 20:05, Ferenc Kovacs wrote:

2013.11.20. 17:42, "Terry Ellison" <ellison.terry@gmail.com <mailto:ellison.terry@gmail.com>> ezt írta: 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?" What I would like is some feedback / guidance / discussion on the general architectural issue which underlies the reasons for these bugs occuring in the first place.... Bumping the thread and ccing Joe as we were having a similar discussion about the internal naming of the inner classes proposed by him. Thanks for bumping this.

Yup, the current build_runtime_defined_function_key() algo builds the function name on the file name (closures) / the class/function name and LANG_SCNG(yy_text) -- that is the memory address of the class / function token being compiled. This is for closures, inner classes or any class or function within a conditional block. I guess this is on the assumption that this will be unique, but as I have shown in #64291 and #65915 it is fairly straight forward to construct test cases where this assumption is invalid, and arjan seems to be hitting this in real-life OPcache cases. My patch at least demonstrates that having a unique key will fix this, but as submitted (adding a simple sequence) this will only reduces this occurrence. However strictly, this could still fail in OPcache because of sequence duplication across forked PHP children. We really need a UUID-style element in the key say based on the creation microtime (or alternatively start time of request + sequence within request) and PID. Regards Terry