Add warning to log, when fpm socket was not registered on the expected path #11066

JoshuaBehrens · 2023-04-12T23:07:35Z

I decided it to be a warning, because you can still run against the newly created socket file but fail it for the configuration test. It felt right to do it like that.

Where am I coming from?

I have an experiments folder deep down in a folder structure on my work machine (/home/name/projects/delete-to-free-storage/experiments). In there I cloned a project where I wanted to try out devenv for a part in a subfolder (/home/name/projects/delete-to-free-storage/experiments/foobar/php-web). I set up devenv, it uses process-compose to orchestrate php-fpm for me and it places the php-fpm socket file in a project specific sub directory (/home/name/projects/delete-to-free-storage/experiments/foobar/php-web/.devenv/state/php-fpm/web). When I opened up the nginx served page, all I get is a white page and nginx prints into the log dialing backend: dial unix /home/name/projects/delete-to-free-storage/experiments/foobar/php-web/.devenv/state/php-fpm/web.sock: connect: invalid argument.

So now try to debug this. You have multiple sources for this error (devenv, process-compose, nix, a nix package for php, php itself, nginx, a nix package for nginx). I was looking into to this with @jochenmanz for several days. Our question was: "Why is the file extension missing?" We were looking through the different tools but no solution

Just today I just received the message "What OS are you on and did you know that socket file names are limited in length?" After just moving the project into a directory with a shorter path, we saw, that it was working and the socket file had the .sock extension as expected. "the extension .sock was missing" is just a coincidence in after a string length cut. So we found the issue it was on binding the socket. We found some checks against in php for every socket access. But for some reason not for php-fpm. The socket will just get cut off. So I decided to fix that so have other people minds less stressed :)

bukka

It makes sense but needs some tests.

sapi/fpm/fpm/fpm_unix.c

sapi/fpm/fpm/fpm_sockets.c

sapi/fpm/fpm/fpm_unix.c

JoshuaBehrens · 2023-04-16T19:54:47Z

@bukka I am happy to write some tests for it. Can you give me a reference test that I can copy or extend for my feature?

bukka · 2023-04-17T11:05:24Z

Check this one out: https://github.com/php/php-src/blob/414f71a90254cc33896bb3ba953f979f743c198c/sapi/fpm/tests/socket-uds-numeric-ugid-nonroot.phpt . It should hopefully give you some idea. You will need to create a custom address so something with a long suffix should hopefully do.

JoshuaBehrens · 2023-04-27T11:59:49Z

Hi @bukka I created two tests. One for actually running fpm and one for the test. I added a section in the upgrade notes. I am up to any wording changes :) With this I noticed I affected other tests as well. For example I broke bug68591-conf-test-listen-group.log:

ERROR: [pool unconfined] cannot get gid for group 'aaaaaa': %s
ERROR: FPM initialization failed
Done
---- ACTUAL OUTPUT
ERROR: [pool unconfined] cannot bind to UNIX socket '/var/folders/7j/lfxr9fd12tn1lcfj3bl9f_7r0000gn/T/nix-shell.XXXXXX.mu6K44Ew/5cf922da-bug68591-conf-test-listen-group.9008.sock' as path is too long (found length: 125, maximal length: 104)
ERROR: FPM initialization failed

I was able to unbreak it but therefore I changed code in the Tester class. This might affect more than I can grasp right now but it fights the fact on Mac the temp folder is not /tmp and therefore not a shorter but rather longer option for a socket file path.

Sidenote: Are you interested in feedback on my experience as first time contributor?

iluuu1994 · 2023-04-27T12:03:12Z

Sidenote: Are you interested in feedback on my experience as first time contributor?

We're definitely interested in making contributions a more pleasant experience for everyone. 🙂 Let me know if there's something we can improve.

JoshuaBehrens · 2023-04-27T13:19:02Z

@iluuu1994 thank you :) for taking the feedback:
I am not sure where I would find that info in the first place but I did not really find a guide how to make a pull request for this project. I had to figure out which configure options worked out for my development setup. I run make test a lot and had to wait a lot. At some time during writing tests I stumbled upon https://qa.php.net/ which helped me to find minimal configure options in the cron example and run a single test. The page helped me a lot although the preamble is basically: "you have no idea how to write c? No worries, testing is simple." Thrive to investigation and digging into it helped me to stay on this topic. I am not sure whether this is the contributor experience you want to provide or whether this is intentional gate keeping to ensure a certain level of expertise. Likely I was just searching for the wrong terms and the search engine did not gave me helpful results. Maybe there is also an entrypoint for my kind of persona missing and therefore I could not find it. I hope this is helpful feedback :)

iluuu1994 · 2023-04-27T13:23:20Z

There's https://www.phpinternalsbook.com/, which is likely the most up to date resource atm. But that doesn't help much when it's linked nowhere 😄 Adding a basic contributors guide is certainly a good idea.

JoshuaBehrens · 2023-04-27T13:40:21Z

Without having a detailed look into it it looks promising :D I will give it a go next time I am up to change something

iluuu1994 · 2023-04-28T12:17:37Z

@JoshuaBehrens I created a PR here: #11155 Feel free to comment if you have any other suggestions!

bukka

The C part looks good. Just the tests need some changes (mostly smaller one - it looks good in general).

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt

bukka · 2023-05-19T12:54:41Z

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt

+$files = [];
+
+foreach (glob($socketFilePrefix . '*') as $file) {
+    $files[] = $file;
+}


NIT: Can't you just do this:

Suggested change

$files = [];

foreach (glob($socketFilePrefix . '*') as $file) {

$files[] = $file;

}

$files = glob($socketFilePrefix . '*');

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt

bukka · 2023-05-19T12:58:22Z

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt

+$socketFile = $socketFilePrefix . '-that-is-too-long-0000-1111-2222-3333-4444-5555-6666-7777-8888-9999-aaaa-bbbb-cccc-dddd-eeee-ffff-gggg-hhhh-iiii.sock';
+
+if (is_file($socketFile)) {


is this actually correct if it creates trimmed file? It should probably glob it and delete the trimmed file, right?

is this actually correct if it creates trimmed file? this is what happens already today. I just try to make one aware of it so one can bind to the right socket

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-for-test.phpt

sapi/fpm/tests/tester.inc

UPGRADING

JoshuaBehrens · 2023-05-20T13:10:47Z

I rebased and amend the code in previous commits so it is a bit cleaner to read the history.

I suggest an UPGRADE notice like:

- Parsing of invalid values will now trigger a warning when this was silently
  ignored before. Interpretation of these values is not changed, for
  backwards compatibility. Testing configuration can fail though.
  This affects the following settings:
  . {fpm_pool}.listen

bukka

Apology for slightly late review. Completely forgot about this when doing FPM stuff last month... Just some minor suggestions and one question added.

sapi/fpm/tests/tester.inc

sapi/fpm/fpm/fpm_unix.c

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt

JoshuaBehrens · 2023-06-29T07:52:26Z

I rebased, amend previous commits to the given feedback.

I hope I got the helper for the socket directory into a more welcome solution. I need feedback on the log type so I can amend this as well :)

This might happen if the UDS length limit is exceeded. Co-authored-by: Jakub Zelenka <[email protected]> Closes phpGH-11066

bukka · 2023-07-15T13:28:14Z

So I have been testing this and it was actually failing due to incorrect test (the one for FPM test error check) which I needed to rewrite. Also pipeline build was failing because it requires using %zu when formatting size_t which I also fixed. I did some few other things as well (CS, test formatting and that extra test check). I have done all those changes myself as the feature freeze is on Tuesday so it can get to 8.3. After that pipeline was green so it is all merged now! Thanks for you work!

JoshuaBehrens · 2023-07-16T14:33:37Z

Thank you for the collaborative work and investment to get it in before the feature freeze <3

andypost · 2023-07-18T00:45:03Z

Trying to build for Alpinelinux getting the test to fail

TEST 16510/16569 [sapi/fpm/tests/socket-uds-too-long-filename-start.phpt]
========DIFF========
+ Socket files were not found.
+
+ Warning: Undefined array key 0 in /builds/alpine/aports/testing/php83/src/php-src-0b0cec5b8a920bafa07dd702950a092a8811596d/sapi/fpm/tests/socket-uds-too-long-filename-start.php on line 41
     Done
========DONE========
[FAIL FPM: UNIX socket filename is too for start [sapi/fpm/tests/socket-uds-too-long-filename-start.phpt]

andypost · 2023-07-18T02:23:56Z

Running within /builds/alpine fails as well as /mnt0123456789 - so length to source should be less then 88 chars

andypost · 2023-07-18T02:36:21Z

That's because socket maxpath is 108 chars

JoshuaBehrens requested a review from bukka as a code owner April 12, 2023 23:07

github-actions bot added the SAPI: fpm label Apr 12, 2023

bukka requested changes Apr 16, 2023

View reviewed changes

sapi/fpm/fpm/fpm_unix.c Outdated Show resolved Hide resolved

sapi/fpm/fpm/fpm_unix.c Outdated Show resolved Hide resolved

sapi/fpm/fpm/fpm_sockets.c Outdated Show resolved Hide resolved

sapi/fpm/fpm/fpm_unix.c Outdated Show resolved Hide resolved

bukka requested changes May 19, 2023

View reviewed changes

bukka reviewed May 19, 2023

View reviewed changes

UPGRADING Outdated Show resolved Hide resolved

JoshuaBehrens force-pushed the feature/cut-fpm-socket-path-warning branch from 70c3036 to 4396f3d Compare May 20, 2023 13:10

bukka reviewed Jun 15, 2023

View reviewed changes

sapi/fpm/tests/tester.inc Outdated Show resolved Hide resolved

sapi/fpm/fpm/fpm_unix.c Outdated Show resolved Hide resolved

sapi/fpm/tests/gh11066-fpm-unix-socket-too-long-filename-but-starts-anyway.phpt Outdated Show resolved Hide resolved

JoshuaBehrens force-pushed the feature/cut-fpm-socket-path-warning branch from 4396f3d to 0cb3074 Compare June 29, 2023 07:51

Warn when fpm socket was not registered on the expected path

4aa1bf9

This might happen if the UDS length limit is exceeded. Co-authored-by: Jakub Zelenka <[email protected]> Closes phpGH-11066

bukka force-pushed the feature/cut-fpm-socket-path-warning branch from 0cb3074 to 4aa1bf9 Compare July 15, 2023 12:11

bukka closed this in 08b5777 Jul 15, 2023

JoshuaBehrens deleted the feature/cut-fpm-socket-path-warning branch September 25, 2023 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning to log, when fpm socket was not registered on the expected path #11066

Add warning to log, when fpm socket was not registered on the expected path #11066

JoshuaBehrens commented Apr 12, 2023

bukka left a comment

JoshuaBehrens commented Apr 16, 2023

bukka commented Apr 17, 2023

JoshuaBehrens commented Apr 27, 2023

iluuu1994 commented Apr 27, 2023

JoshuaBehrens commented Apr 27, 2023 •

edited

Loading

iluuu1994 commented Apr 27, 2023

JoshuaBehrens commented Apr 27, 2023

iluuu1994 commented Apr 28, 2023

bukka left a comment

bukka May 19, 2023

bukka May 19, 2023

JoshuaBehrens May 20, 2023

JoshuaBehrens commented May 20, 2023

bukka left a comment

JoshuaBehrens commented Jun 29, 2023

bukka commented Jul 15, 2023

JoshuaBehrens commented Jul 16, 2023

andypost commented Jul 18, 2023 •

edited

Loading

andypost commented Jul 18, 2023

andypost commented Jul 18, 2023

		$socketFile = $socketFilePrefix . '-that-is-too-long-0000-1111-2222-3333-4444-5555-6666-7777-8888-9999-aaaa-bbbb-cccc-dddd-eeee-ffff-gggg-hhhh-iiii.sock';

		if (is_file($socketFile)) {

Add warning to log, when fpm socket was not registered on the expected path #11066

Add warning to log, when fpm socket was not registered on the expected path #11066

Conversation

JoshuaBehrens commented Apr 12, 2023

Where am I coming from?

bukka left a comment

Choose a reason for hiding this comment

JoshuaBehrens commented Apr 16, 2023

bukka commented Apr 17, 2023

JoshuaBehrens commented Apr 27, 2023

iluuu1994 commented Apr 27, 2023

JoshuaBehrens commented Apr 27, 2023 • edited Loading

iluuu1994 commented Apr 27, 2023

JoshuaBehrens commented Apr 27, 2023

iluuu1994 commented Apr 28, 2023

bukka left a comment

Choose a reason for hiding this comment

bukka May 19, 2023

Choose a reason for hiding this comment

bukka May 19, 2023

Choose a reason for hiding this comment

JoshuaBehrens May 20, 2023

Choose a reason for hiding this comment

JoshuaBehrens commented May 20, 2023

bukka left a comment

Choose a reason for hiding this comment

JoshuaBehrens commented Jun 29, 2023

bukka commented Jul 15, 2023

JoshuaBehrens commented Jul 16, 2023

andypost commented Jul 18, 2023 • edited Loading

andypost commented Jul 18, 2023

andypost commented Jul 18, 2023

JoshuaBehrens commented Apr 27, 2023 •

edited

Loading

andypost commented Jul 18, 2023 •

edited

Loading