thread-safe and thread-local setlocale()

[henderkes]

It's documented at https://www.php.net/manual/en/function.setlocale.php that the setlocale php function is not thread locale (and actually, not thread safe, because the C setlocale function is MT-unsafe). This leads to very unexpected behaviour in FrankenPHP.

I would therefore like to propose changing setlocale to use POSIX uselocale instead.
Unfortunately, because querylocale is only available on BSD, this adds a lot of string caching logic to let users query for current locales, as I'm not aware of a different way. Perhaps you can give me some pointers to avoid this logic.

The logic for windows remains unchanged.

See: php/frankenphp#1941

Given the following PHP code to simulate different FrankenPHP requests:

<?php
use parallel\Runtime;

setlocale(LC_ALL,'C');
echo "main@start   : ".setlocale(LC_ALL,0)." | dp=".localeconv()['decimal_point']."\n";

$f = (new Runtime())->run(function () {
    setlocale(LC_ALL,'de_DE.UTF-8');
    echo "worker@set   : ".setlocale(LC_ALL,0)." | dp=".localeconv()['decimal_point']."\n";
});

echo "main@after   : ".setlocale(LC_ALL,0)." | dp=".localeconv()['decimal_point']."\n";

$f->value();

Currently:

[m@M bin]$ php -d "extension=/home/m/static-php-cli/buildroot/modules/parallel.so" localetest.php 
main@start   : C.UTF-8 | dp=.
worker@set   : de_DE.UTF-8 | dp=,
main@after   : de_DE.UTF-8 | dp=,

Expected, after proposed implementation:

[m@M bin]$ ./php -d "extension=/home/m/static-php-cli/buildroot/modules/parallel.so" localetest.php 
main@start   : C.UTF-8 | dp=.
worker@set   : de_DE.UTF-8 | dp=,
main@after   : C.UTF-8 | dp=.

[Girgias]

Frankly, I'd rather try to push people away from using setlocale() by recommending them to use ICU related functionality rather than relying on the OS locales behaving properly.

[Girgias]

Suggested change

	} else if (zend_string_equals_cstr(loc, ZSTR_VAL(ret_str), ZSTR_LEN(ret_str))) {
	} else if (zend_string_equals(loc, ret_str)) {

[henderkes]

I would also be fine with deprecating setlocale(), would avoid this string mess as well. I just don't believe there should be functions in a ZTS build that can crash the process.

[krakjoe]

The change over to uselocale looks like a reasonable approach to this specific problem, I'm not a fan of the idea of deprecating setlocale on the basis that it's not actually broken today anymore than it was last year, or the year before ... There are safe ways to use it in a legitimate way - it's safe to call setlocale in preload for example.

A softer approach might be changing the documentation to highlight the problems, and preferred solution(s).

More broadly, this is a symptom of taking code which - like basically all code written for the last twenty years - assumes it will be executed in a pre-fork environment.

FrankenPHP innovations at the frontend (server related stuff) do not require a shared address space, similarly innovation at the backend (ie, workers) also do not require a shared address space.

I spent a little bit of time digging into the frankenphp code, and I notice that you have complete control over how threads are created.

Here is an area of interesting research for you (this is just an idea): I suggest you go a level down from pthread_create and look at the clone API. In principle, it is possible for FrankenPHP to implement the pre-fork model this way, and so restore stability and the correctness of everybody's assumptions (and avoid a whole class a bugs, like this one). Obviously this would require that you forgo channel based communications between go and php threads, but it's highly unlikely that these are providing performance characteristics that you cannot match with IPC and SHM. If you can make this work, it looks like a better threading model for frankenphp, that allows you to keep all of your innovations, and avoid essentially all of the pitfalls of using php threaded.

[henderkes]

A softer approach might be changing the documentation to highlight the problems, and preferred solution(s).

I think that's a great idea regardless, but if php itself never tells you what's wrong, it's unlikely that users would open the documentation. Therefore I believe that setlocale should either be "fixed", or deprecated. Sure, not all users look at deprecation logs either, but that'd be on them.

FrankenPHP innovations at the frontend (server related stuff) do not require a shared address space, similarly innovation at the backend (ie, workers) also do not require a shared address space.

You are generally correct! @withinboredom actually just started a discussion about our approach a few days ago and, as I understand it, a long-term goal is still for Go to provide the async backend for when that becomes a possibility - essentially executing php in Goroutines directly.

We could of course hook into all MT-unsafe functions to overwrite them, might even be fairly simple in this case (I'm not very familiar with Go's standard library), but I thought fixing it in php-src would be the better idea, given that it would help mod_php as well.