Fix GH-12778: Windows IIS FastCGI Spin-Up Performance Slowdown

[nielsdos]

The issue isn't limited to Windows only (although locales on Windows are much slower so the impact is higher), I can also see the slowdown on my Linux machine. The cause of the slowdown is 26e4244. In that commit a locale reset routine was implemented that gets called on startup.
The fix from that commit is only necessary when using readline functionality, there are three places that is used:

• phpdbg
• with readline_cli
• readline() php function

So we make sure we only reset the locale when one of these is used, preventing the overhead on normal website-serving use-cases.

I targeted PHP 8.2 here because I find it a bit risky to put this into 8.1 as today is the last day for bugfixes...
I'm also not targeting master because it's a fix for a performance regression.

[iluuu1994]

Sorry for the late review. I suppose other SAPIs and/or extensions could rely on the locale being set, so this is technically a BC break. I don't know whether that's the case in practice...

[nielsdos]

Sorry for the late review.

Wouldn't consider it late, but no worries anyway ;)

I suppose other SAPIs and/or extensions could rely on the locale being set, so this is technically a BC break. I don't know whether that's the case in practice...

Right... Even though this call was introduced in a patch release it has been there for so long that Hyrum's law may apply...
It's even more sad because AFAICT this is only actually needed when libedit is used instead of libreadline.
So WDYT about putting this into master and documenting this in UPGRADING.INTERNALS?

[iluuu1994]

Master is certainly a non-issue. Release branches may be fine too, I just wanted to express the possibility of behavioral changes. FWIW the code looks correct to me, I'm ok with merging this with the very small chance of regression. We should still document this accordingly.

[nielsdos]

Hmm. I kind of want to err on the safe side.
I've rebased this on master, and I've added a note to UPGRADING.INTERNALS.
If after a while in master we have not seen regressions we could backport this to stable releases.
If we do backport I'm not sure where to document it, for master it's obvious it has to go in UPGRADING.INTERNALS, but I guess that document should not be changed on stable branches?

[nielsdos]

Very fittingly, looks like this now fails: Bug #74589 __DIR__ wrong for unicode character, UTF-8 [ext/standard/tests/directory/bug74589_utf8.phpt]. Strangely this test was also in the 8.2 branch but didn't fail when the PR targeted that. I'll try to find some time soon to try and figure out why that is. :/

[bukka]

Looks ok to me for master (assuming all tests get fixed...)

[nielsdos]

I figured out why that one test is failing on master but not on 8.2. The difference in the test is that on master it applies escapeshellarg to the UTF-8 path.
This PR causes a change in how escapeshellarg behaves because that function uses php_mblen internally which just calls mblen from the C library which depends on the locale. So when we don't reset the locale at startup the locale is simply "C" instead of "C.UTF8", causing different behaviour.
I don't know yet how to fix this. We could do the same lazy setlocale trick like I did with readline, but that would require inspecting all places where this php_mblen macro is used. Thoughts?

[nielsdos]

Actually I found https://bugs.php.net/bug.php?id=54391 now, so perhaps there is an issue with the escapeshellarg function where it should not remove characters that it can't decode but simply skip them...

[cmb69]

Actually I found https://bugs.php.net/bug.php?id=54391 now, so perhaps there is an issue with the escapeshellarg function where it should not remove characters that it can't decode but simply skip them...

I'm afraid the whole idea of escapeshellarg() is broken; either you restrict the function as we have it now (it's even worse on Windows, I think), or you risk potential issues. After all, it's not necessary to use escapeshellarg() right before executing some command, but users may call escapeshellarg() in one environment, and then execute the command in a different one. It occurs to me the only sane way to call a command where some arguments are user input is to use proc_open() with an array.

There may be valid cases for escapeshellarg(), though, but I wouldn't change the behavior, but rather let users be explicit about it (by adding some new argument to the function, or maybe even introduce a new function).