Repository navigation
Make setlocale() thread-local in ZTS builds - #25420
felixbuenemann wants to merge 1 commit into
Conversation
|
@henderkes If I read you comment on php/frankenphp#2831 correctly, you are proposing to always use the thread-safe code, even in NTS builds? |
|
Unrelated |
henderkes
left a comment
There was a problem hiding this comment.
Just some initial thoughts
| { | ||
| /* Locales are looked up and created one at a time: the "" locale reads | ||
| * the environment, and concurrent newlocale() calls crash on macOS */ | ||
| tsrm_env_lock(); |
There was a problem hiding this comment.
I'd prefer a lock free implementation where possible.
There was a problem hiding this comment.
Looking up an existing locale no longer takes a lock. It uses an atomic pointer as the head of an append-only list.
A lock is still used for:
- Creating a locale (prevents crashes with cocurrent
newloclae()calls on macOS), with additional testing could maybe be reduced to Darwin and BSD if also affected - Resolving the
""locale, because it reads the environment (preventsgetenv()race)
setlocale() changes the locale of the whole process and isn't thread-safe. In thread-safe builds, a request used the locale set by requests running in other threads, the reset at the end of a request also reset the locale of all other requests, and concurrent calls could crash. zend_setlocale() replaces it. During a request, it switches the current thread to a locale of its own with uselocale(), which the locale dependent functions of the C library use in place of the process locale. Outside of a request, it still changes the process locale, which is the initial locale of all threads. Builds that aren't thread-safe use the same code, so there is a single implementation. Windows already keeps the locale per thread, and systems without uselocale() keep using setlocale(). The locale objects are created once for each combination of locales and shared by all threads. They are never freed: threads keep using them, and macOS crashes when locales are created and freed concurrently. Known locales are looked up without a lock, only creating one takes it. The ENV sections of setlocale_variation4.phpt and setlocale_variation5.phpt had a trailing semicolon, which made the locale "en_US.utf8;". glibc accepts that name in setlocale(), but not in newlocale().
220c655 to
8e394eb
Compare
|
@henderkes The |
|
@henderkes I am unsure what to do about the FreeBSD failure: The problem is that on FreeBSD and DragonFly A FreeBSD specific hack could partially solve it: uselocale(loc);
__runes_for_locale(loc, &limit); /* the limit of this locale */
__mb_sb_limit = limit;But for multiple threads switching locales that is only correct, if all threads use either UTF-8 or non-UTF-8 locales :-( Another approach is to use Or we could stay on Let me know what you think or if you have a better idea. |
In thread-safe builds,
setlocale()changes the locale of the whole process:setlocale()isn't thread-safe.The manual documents the first point as a limitation. Windows doesn't have it: PHP keeps the locale per thread there since 5.3.6 (0778bed) and 5.6.20/7.0.5 (632fc51, bug #71596). This PR does the same for the other platforms, using
newlocale()anduselocale().It takes the approach of #20531, which was closed in favour of a broader ZTS rework. On internals (externals.io/message/128991), a pull request was suggested rather than an RFC.
NTS builds use the same code, so there is a single implementation. There,
setlocale()no longer changes the process locale either.Changes
zend_setlocale()inZend/zend_locale.c. On systems withuselocale(), in ZTS and NTS builds alike, during a request it switches the current thread to a locale of its own. The locale-dependent functions of the C library (printf(),localeconv(),strftime(),strcoll(),nl_langinfo(),mblen()…) use that locale in place of the process locale. Elsewhere it's a macro forsetlocale().setlocale()calls go through it: thesetlocale()function, the reset inPHP_RSHUTDOWN(basic),zend_reset_lc_ctype_locale(),zend_update_current_locale()andNumberFormatter::parse().configure.acchecks foruselocale()andxlocale.h.Behaviour that doesn't change:
!EG(active)),zend_setlocale()changes the process locale as before.Cat the end. That now only affects its own thread, as on Windows.setlocale()returns the same names as the C library'ssetlocale(), including the name ofLC_ALLwhen the categories have different locales. That name has a different format in glibc, musl and macOS/BSD, sosetlocale(LC_ALL, $saved)keeps working.Implementation notes
uselocale().newlocale()andfreelocale()run concurrently. The number of objects is not bounded, though: it grows with the distinct combinations a process sets.tsrm_env_lock(), and so does resolving the""locale, which reads the environment.setlocale()run no new code.setlocale(LC_ALL, NULL): on macOS that call writes to a static buffer, so concurrent calls return garbage.Tests
setlocale_variation6.phpt(new) covers restoring a saved name ofLC_ALLwhen the categories have different locales.setlocale_variation4.phptandsetlocale_variation5.phpthad a trailing semicolon in their--ENV--section, so the locale wasen_US.utf8;. glibc accepts that name insetlocale(), but not innewlocale(). The semicolon is removed.setlocale()edge cases (invalid names,'0','',null, arrays,POSIX, wrong types…), 7 environments (LANG,LC_ALL,LC_NUMERIC, none…) and 15 names ofLC_ALLwith missing, extra or empty parts. The output is identical.ext/standard/tests/strings,ext/standard/tests/general_functions,ext/ctype,ext/pcreandtests/langgive the same results in ZTS and NTS builds on all three systems. TheNumberFormattertests pass on Debian and Alpine. The gettext tests pass in an NTS build on macOS (GNU libintl), as on master.Limitations
setlocale()directly, in an extension or a library, still changes the process locale, and doesn't see the locale of the thread. That now applies to NTS builds too. Extensions should callzend_setlocale()instead (see UPGRADING.INTERNALS).LC_ALLis negative (AIX, as far as I know),zend_setlocale()stays a macro forsetlocale(), because the category table inzend_locale.cis indexed by category.LC_ALLfor different category locales uses the musl format. It still round-trips throughsetlocale().Thread test program (embed SAPI)
Built with
--enable-zts --enable-embed, and linked againstlibphp.