FPM: fastcgi_finish_request supports force close connection param

[Appla]

when using fastcgi keepalive connection and fastcgi_finish_request, potentially deadlock may happens.

for example: process-a request same host after called fastcgi_finish_request, the proxy may using keepalived connection which is serve by same process(process-a), this may causes a dead lock

this PR introduced a new param which let user decide whether to close a [keepalive] connection.

[cmb69]

Thanks for the PR!

Instead of manually modifying fpm_main_arginfo.h, you should modify fpm_main.stub.php instead, and then use build/gen_stub.php to regenerate fpm_main_arginfo.h.

[Appla]

Thanks for the PR!

Instead of manually modifying fpm_main_arginfo.h, you should modify fpm_main.stub.php instead, and then use build/gen_stub.php to regenerate fpm_main_arginfo.h.

Thanks, I will update later

[Appla]

Looks like LINUX_X64_DEBUG_NTS fails due to

DBA CDB handler test [ext/dba/tests/dba_cdb_001.phpt] (info: CDB does not support replace or delete)

any suggestion?

[bukka]

It would be also good if you could create an issue for the actual problem describing in more details how to reproduce the deadlock to see if there might be some other solutions for this as well.

This is probably still useful in any case but I think there should be also some way for administrator to disallow closing of the connection as it might have negative impact on the server performance. That way could be some fpm conf option that would for example allow to disable ability to close the connection.

This should also have a test. It could be done by sending 2 requests in tester and adding some extra check to the client and tester itself. I would need to try it but think it should be doable.

[Appla]

It would be also good if you could create an issue for the actual problem describing in more details how to reproduce the deadlock to see if there might be some other solutions for this as well.
Thanks!

Issue added @ #10335

This is probably still useful in any case but I think there should be also some way for administrator to disallow closing of the connection as it might have negative impact on the server performance. That way could be some fpm conf option that would for example allow to disable ability to close the connection.

Good point, but I think as many exists functions that maybe misused, this usally prevent by CODE checker in my experience. If we do need a new option, I would change this PR later.

This should also have a test. It could be done by sending 2 requests in tester and adding some extra check to the client and tester itself. I would need to try it but think it should be doable.

I am not familiar with this but I would try when I get free.

[Appla]

I just tried writing a PHP for this MR, but it seems not possible without a standalone proxy like nginx to do this for me.

the core part is that the PROXY will reuse the process bound connection based on the protocol perspective which the process will not handle connection under normal circumstances.

any suggestion?

[bukka]

The fcgi test client has keep a live support so it should be possible. I might take a look next months if it doesn't work for you.

[Appla]

The fcgi test client has keep a live support so it should be possible. I might take a look next months if it doesn't work for you.

I tried it, but I don't know how to simulate the situation I described above using the keep-alive functionality of the client based on the pfsockopen implementation.
looking forward to your try
Thanks.

[bukka]

I just went through my priorities and there are bunch of bugs that I need to look at first so this might take few months at least.

[Appla]

I just went through my priorities and there are bunch of bugs that I need to look at first so this might take few months at least.

Any updates about this?

[bukka]

This is actually next on my FPM priorities (this and another keep alive things). I'm just starting to look #10335 and will see if there's anything else we could do about this issue. It might take me a bit of time as my time is currently a bit limited and split between some other things.

[kkmuffme]

I think the new param should be removed again and this should be the default behavior, as currently this behavior has security implications, because it causes #18275