Skip to content

Fix GH-18956: PHP-FPM Process Count Inconsistencies #19191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jul 20, 2025

This fixes incorrect decrement of the active number of processes. This was done in accepting stage before incrementing which is problematic if there are already some running processes as it decrements their number first and result in incorrect total (lower than the actual number).

In addition it also fixes GH-14212 as accept is done just once for keepalive connection so in this case the number of active connection just increasing with each request.

@bukka bukka changed the base branch from master to PHP-8.3 July 20, 2025 16:37
This fixes incorrect decrement of the active number of processes. This
was done in accepting stage before incrementing which is problematic if
there are already some running processes as it decrements their number
first and result in incorrect total (lower than the actual number).

In addition it also fixes phpGH-14212 as accept is done just once for
keepalive connection so in this case the number of active connection
just increasing with each request.

Closes phpGH-19191
@bukka
Copy link
Member Author

bukka commented Jul 20, 2025

This needs more testing - I will go only for WST test as PHP test is too tricky for this and would not most likely work in the pipeline anyway.

bukka added a commit to bukka/php-src that referenced this pull request Jul 30, 2025
This fixes incorrect decrement of the active number of processes. This
was done in accepting stage before incrementing which is problematic if
there are already some running processes as it decrements their number
first and result in incorrect total (lower than the actual number).

In addition it also fixes phpGH-14212 as accept is done just once for
keepalive connection so in this case the number of active connection
just increasing with each request.

Closes phpGH-19191
@bukka
Copy link
Member Author

bukka commented Jul 30, 2025

So I have been testing and debugging this. At the end the best way to see what's going on was adding some extra logs which I have in this debug branch: master...bukka:php-src:fpm_gh18956_request_active_decrement_debug .

If it's done without keepalive, a single request call looks like following:

NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=8, type=1)
NOTICE: fcgi reading 2 (len=8, type=1, id=1)
NOTICE: req accepted: 0x5695f037fbc0
NOTICE: finished
NOTICE: req shutdown: 0x5695f037fbc0
NOTICE: scoreboard active--
NOTICE: prev active: 1
NOTICE: new active: 0
NOTICE: prev active: 0
NOTICE: new active: 0

But with keepalive (fastcgi_keep_conn on;), the situation is a bit different

NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=8, type=1)
NOTICE: fcgi reading 2 (len=8, type=1, id=1)
NOTICE: req accepted: 0x5695f037fd50
NOTICE: req shutdown: 0x5695f037fd50
NOTICE: scoreboard active--
NOTICE: prev active: 1
NOTICE: new active: 0
NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=0, type=5)
NOTICE: fcgi reading end 1
NOTICE: finished
NOTICE: prev active: 1
NOTICE: new active: 1

The problem is that there are two on_read calls because keepalive means that FPM (or more fastcgi.c) does second loop but if nginx gets single non keepalive http request, the second read is just 0 so FPM closes connection.

In this case it might seem logical to reduce active in fpm_request_finished (on_close hook) except it wouldn't work for non keepalive. Unfortunately there is currently no way to differentiate it in fpm_request_finished. Also on_close can be called in couple of other cases so it would need additional checking.

I will need to think about it more and see if there is some clean solution. Alternatively there might be some hacky solution to do that but it will require more checking as well. I will get this sorted - it will just require a bit of time to get right and properly test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FPM Status: active processes greater than pm.max_children
1 participant