Updated the stream_select() function so that it supports bigger file … #14452

frodeborli · 2024-06-03T10:00:27Z

Updated the stream_select() function so that it supports bigger file descriptor numbers without recompiling PHP on non-Windows OS, by passing the select() system call dynamically generated fd_sets with a custom fd_bigset struct type.

The select() system call supports unlimited numbers of file descriptors technically, and so should all BSD derived unix operating systems. The perceived limitation of the select() system call to support only 1024 file descriptors is because of the fd_set structure being prebuilt to FD_SETSIZE length, but the kernel supports unlimited.

The new version of this function allocates memory on the heap to build an fd_bigset which is passed to the select() system call, and then frees the structures.

I decided to leave the functions stream_array_to_fd_set and stream_array_from_fd_set functions in streamfuncs.c in case somebody is using that.

I did not change the php_select macro, only the stream_select function.

…descriptor numbers without recompiling PHP on non-Windows OS, by passing the select() system call dynamically generated fd_sets with a custom fd_bigset struct type

…ow if those functions could be referenced by other extensions.

frodeborli · 2024-06-03T10:08:43Z

The testing complained that I had left behind two functions which I didn't dare to remove in case somebody else was using them. I have updated the pr.

…rent architecture than what I developed this on.

frodeborli · 2024-06-03T12:33:10Z

I'm not sure why the tests timed out or failed; it does not seem to be related to my patch.

arnaud-lb

Big +1 on the general idea. This fixes a long standing issue with stream_select() not supporting file descriptors above 1024.

@bukka @devnexen wdyt?

Maybe we can remove the --enable-fd-setsize configure flag as well. I believe it's a no-op in glibc for some time now.

ext/standard/streamsfuncs.c

mvorisek · 2024-06-03T13:41:26Z

ext/standard/streamsfuncs.c

@@ -756,115 +782,132 @@ static int stream_array_emulate_read_fd_set(zval *stream_array)
 /* }}} */



Can this be tested from useland when more than 1024 file descriptors are created/used?

Also when there is less than 1024, but at least one has a number > 1024

@mvorisek I used this code to test:

<?php // Create a large number of file descriptors $files = []; $r = $w = $e = []; for ($i = 0; $i < 20000; $i++) { $fileName = tempnam(sys_get_temp_dir(), 'testfile'); $fp = fopen($fileName, 'r'); if ($fp === false) { die("Failed to open file: $fileName\n"); } $files[] = ['name' => $fileName, 'handle' => $fp]; $r[] = $fp; } // Check the number of file descriptors $res = stream_select($r, $w, $e, 0, 0); var_dump($res, $r); // Clean up foreach ($files as $file) { fclose($file['handle']); unlink($file['name']); }

A function doing dup2() can probably be added to zend_test, allowing to zend_test_dup2(1, 9999).

@TimWolla Is zend_test a mechanism to add special functions to the PHP runtime during testing? I think it would be nice to have zend_test_get_fd($resource), but perhaps that already exists?

@frodeborli

Is zend_test a mechanism to add special functions to the PHP runtime during testing?

Yes, exactly. It is meant to provide helpers to test stuff that would otherwise be untestable. You can find it in https://github.com/php/php-src/tree/master/ext/zend_test and can enable it at build time using --enable-zend-test.

…est file descriptor id and allocates a correct size.

…find.

frodeborli · 2024-06-03T15:58:03Z

I will look over the PR thoroughly later today or tomorrow morning.

Can anybody tell me how to build this in a stricter mode? Several commits could have been avoided by me if only my environment would let me know about unused functions etc, instead of waiting for the github CI process to check for me.

devnexen · 2024-06-03T16:06:45Z

I will look over the PR thoroughly later today or tomorrow morning.

Can anybody tell me how to build this in a stricter mode? Several commits could have been avoided by me if only my environment would let me know about unused functions etc, instead of waiting for the github CI process to check for me.

Speaking of CI, you can get inspiration from it if you like.

ext/standard/streamsfuncs.c

ramsey · 2024-06-04T05:02:51Z

The only feedback I would give is to please update your editor to respect the .editorconfig file provided with PHP. https://editorconfig.org

It looks like you've converted a lot of tab characters to spaces, so the diff is much bigger than it should be.

ext/standard/streamsfuncs.c

frodeborli · 2024-06-04T14:38:22Z

I am having trouble because stream wrappers emit warnings without respecting the silent cast argument of php_stream_cast...

So a phpt test does not generate the exact same output anymore after my patch.

arnaud-lb · 2024-06-04T15:36:32Z

The cause of the extra warnings in ext/standard/tests/file/userstreams_002.phpt appear to be that max_fd is never equal to -1 here.

ext/standard/streamsfuncs.c

frodeborli · 2024-06-05T13:56:12Z

Now it seems all tests succeeded, except Windows 64 bit. Not sure how to proceed. I think the php_select macro refers to a win32 implementation which is meant to be compatible with select() in Linux and therefore should work.

ext/standard/streamsfuncs.c

withinboredom · 2024-06-06T18:28:17Z

ext/standard/streamsfuncs.c


-	if (!PHP_SAFE_MAX_FD(max_fd, max_set_count)) {
+	if (max_set_count == 0) {


Are we missing FD_BIGSET_FREE before returning?

I'm not sure where you mean; I think FD_BIGSET_FREE is everywhere it is needed now, but there was a few places they were missing before the last push.

We've already called FD_BIGSET_ZERO by this point, so the memory is allocated. On this line, we are about to return without freeing that memory; thus I think we need to free it.

That being said, I'm not quite sure how we would get here.

ext/standard/streamsfuncs.c

… the PR from github.

frodeborli · 2024-06-07T14:47:15Z

in any case it would fall back to the current behavior Sendt fra Outlook for iOS<https://aka.ms/o0ukef>

________________________________ Fra: Rob Landers ***@***.***> Sendt: Friday, June 7, 2024 4:36:36 PM Til: php/php-src ***@***.***> Kopi: Frode Børli ***@***.***>; Author ***@***.***> Emne: Re: [php/php-src] Updated the stream_select() function so that it supports bigger file … (PR #14452) @withinboredom commented on this pull request.

________________________________ In ext/standard/streamsfuncs.c<#14452 (comment)>:

+ FD_BIGSET_ZERO(&rfds, max_fds);

+ FD_BIGSET_ZERO(&wfds, max_fds); + FD_BIGSET_ZERO(&efds, max_fds); See man sysconf<https://www.man7.org/linux/man-pages/man3/sysconf.3.html>: The return value of sysconf() is one of the following: On error, -1 is returned and errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is set to indicate the error (for example, EINVAL, indicating that name is invalid). If name corresponds to a maximum or minimum limit, and that limit is indeterminate, -1 is returned and errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is not changed. (To distinguish an indeterminate limit from an error, set errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> to zero before the call, and then check whether errno<https://www.man7.org/linux/man-pages/man3/errno.3.html> is nonzero when -1 is returned.) If name corresponds to an option, a positive value is returned if the option is supported, and -1 is returned if the option is not supported. Otherwise, the current value of the option or limit is returned. This value will not be more restrictive than the corresponding value that was described to the application in <unistd.h> or <limits.h> when the application was compiled. So, theoretically, if uname -n unlimited were possible, you could also receive a -1 too. AFAIK, it isn't possible to set the open file limit to unlimited though. (I could be wrong, but I'm 95% sure). — Reply to this email directly, view it on GitHub<#14452 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AARRLUDB2NEIVS4KIIK75JTZGHAPJAVCNFSM6AAAAABIWFGFUGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBUG42TENJUGQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

ext/standard/streamsfuncs.c

…se an extremely high ulimit -n is configured.

frodeborli · 2024-06-27T12:37:17Z

The failing tests seem to relate to jit, not stream_select?

frodeborli · 2024-06-27T17:24:22Z

I think it will be very rare with file descriptors above 131k, at least for the next couple of years. Perhaps the best solution is to reallocate on the fly if we see fds above 131k and use a single pass. 131k is extremely high for a single process so I think it will be rare to encounter it.

…d_bigset structure whenever needed

ext/standard/streamsfuncs.c

arnaud-lb · 2024-07-02T16:14:24Z

ext/standard/streamsfuncs.c

+    size_t largest = rfds.size;
+    if (largest < wfds.size) largest = wfds.size;
+    if (largest < efds.size) largest = efds.size;


I think we can use max_fd to directly compute the largest size?

Something like:

while (max_fd > rfds.size) fd_bigset_double_size(&rfds);

I was just not confident about possibly off by one error here...

We can use the same condition as in FD_BIGSET_ENSURE_CAPACITY(), or pre-compute the largest size with:

size_t num_fds = max_fd + 1; size_t largest = (num_fds + 7) / 8;

ext/standard/streamsfuncs.c

frodeborli · 2024-07-02T20:27:10Z

@arnaud-lb Thank you for your detailed review. I'm merging the latest php-src and pushing an updated PR with all your suggestions.

I was looking into Windows which apparently only has FD_SETSIZE 64 by default, and it uses an entirely different fd_set structure. Not sure how this is handled inside PHP and if my approach breaks it? I've not got a Windows C development environment, but I can look into it if you can give some hints perhaps?

arnaud-lb · 2024-07-03T13:18:18Z

Oh right, it looks like this will break on Windows. We use WinSock select() according to this comment, so fd_set is as defined here: https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-fd_set on Windows. It represents an array of handles rather than a bitset, and the limit is the number of watched fds rather than the max fd value.

We can use the same strategy as for other platforms, but use the fd count to decide whether to grow the set. We have to define the FD_BIGSET_ macros differently on windows:

#ifdef PHP_WIN32

typedef struct {
    uint32_t capacity;
    fd_set set;
} fd_bigset;

#define FD_BIGSET_ENSURE_CAPACITY(fd, set) \
    if (UNEXPECTED(fd.set.count == fd.capacity)) { \
        fd_bigset_double_size(set); \
    }

// other FD_BIGSET macros for windows

#else 

// FD_BIGSET macros for non-windows

#endif

However there is a (non-enforced) hard limit of MAXIMUM_WAIT_OBJECTS (64) non-socket fds in win32/select.c. I would suggest to return a failure from php_select() when there are more than MAXIMUM_WAIT_OBJECTS non-socket fds.

php_select() copies fd_set structs by assignment (fd_set sock_read, aread; ...; aread = sock_read;). This will not work with custom sized sets, so we need to replace these with memcpy()s.

It would make sense to do part of this work later in a separate PR, as long as this PR doesn't break windows. E.g. don't support more than FD_SETSIZE fds for now on Windows.

I've not got a Windows C development environment, but I can look into it if you can give some hints perhaps?

I sometimes use a Windows VM from https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/ (The VirtualBox ones work well. If you use virt-manager you may be able to convert them to KVM but that's not trivial.). This page describes how to build on windows: https://wiki.php.net/internals/windows/stepbystepbuild_sdk_2

bukka

I will take a proper look later but could you please in the meantime fix the coding style.

bukka · 2024-07-05T10:21:55Z

ext/standard/streamsfuncs.c

+typedef struct {
+    char *fds_bits;
+    size_t size;
+} fd_bigset;
+#define FD_BIGSET_ENSURE_CAPACITY(fd, set) \
+    if (UNEXPECTED(((fd) / 8 >= (set)->size))) { \
+        fd_bigset_double_size(set); \
+    }
+#define FD_BIGSET_ZERO(set, num_fds) do { \
+    (set)->size = (num_fds + 7) / 8; \
+    (set)->fds_bits = (char *)ecalloc((set)->size, sizeof(char)); \
+} while (0)
+#define FD_BIGSET_SET(fd, set) do { \
+	FD_BIGSET_ENSURE_CAPACITY(fd, set) \
+	(set)->fds_bits[(fd) / 8] |= (1 << ((fd) % 8)); \
+} while (0)
+#define FD_BIGSET_ISSET(fd, set) (((fd) / 8 < (set)->size) ? ((set)->fds_bits[(fd) / 8] & (1 << ((fd) % 8))) : 0)
+#define FD_BIGSET_CLR(fd, set) do { \
+    if ((fd) / 8 < (set)->size) { \
+        (set)->fds_bits[(fd) / 8] &= ~(1 << ((fd) % 8)); \
+    } \
+} while (0)
+#define FD_BIGSET_FREE(set) do { \
+    if ((set)->fds_bits) { \
+        efree((set)->fds_bits); \
+        (set)->fds_bits = NULL; \
+    } \
+    (set)->size = 0; \
+} while (0)
+
+/* {{{ Function to double the size of an fd_bigset */
+static void fd_bigset_double_size(fd_bigset *set) {
+	ZEND_ASSERT(set && set->fds_bits);
+
+    size_t old_size = set->size;
+    size_t new_size = old_size * 2;


Can you please fix coding style - we use tabs, not spaces. This is making the diff bigger and harder to review.

bukka · 2024-07-05T10:23:28Z

btw I have a real Windows env so I can test it then too.

Updated the stream_select() function so that it supports bigger file …

b570514

…descriptor numbers without recompiling PHP on non-Windows OS, by passing the select() system call dynamically generated fd_sets with a custom fd_bigset struct type

frodeborli requested a review from bukka as a code owner June 3, 2024 10:00

github-actions bot added the Extension: standard label Jun 3, 2024

Removed unused functions. Unsure if this is correct, since I don't kn…

166a0e4

…ow if those functions could be referenced by other extensions.

frodeborli and others added 2 commits June 3, 2024 12:10

Merge branch 'php:master' into stream_select_unlimited

e3edadb

Fix bug which was found by CI on php-src when commiting using a diffe…

f8c53e5

…rent architecture than what I developed this on.

arnaud-lb reviewed Jun 3, 2024

View reviewed changes

mvorisek reviewed Jun 3, 2024

View reviewed changes

frodeborli added 2 commits June 3, 2024 13:52

Fixed mistake from lazyness. The function now properly finds the larg…

5631e3b

…est file descriptor id and allocates a correct size.

Apparently the CI on github finds stuff my building process does not …

581a8d5

…find.

withinboredom reviewed Jun 3, 2024

View reviewed changes

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

withinboredom reviewed Jun 3, 2024

View reviewed changes

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

ext/standard/streamsfuncs.c Show resolved Hide resolved

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

withinboredom reviewed Jun 4, 2024

View reviewed changes

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

Problems

b93d754

frodeborli added 2 commits June 4, 2024 22:54

Updated code setting the max_fds based on the ulimit for the process.

ae8d2ec

Fixed order of some input tests that caused testing to fail

5fed836

withinboredom reviewed Jun 5, 2024

View reviewed changes

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

ext/standard/streamsfuncs.c Show resolved Hide resolved

Merge branch 'php:master' into stream_select_unlimited

9f98fe4

withinboredom reviewed Jun 6, 2024

View reviewed changes

Refactored code and improved safey of macros according to comments on…

7c07b77

… the PR from github.

withinboredom reviewed Jun 7, 2024

View reviewed changes

ext/standard/streamsfuncs.c Show resolved Hide resolved

arnaud-lb reviewed Jun 10, 2024

View reviewed changes

ext/standard/streamsfuncs.c Outdated Show resolved Hide resolved

frodeborli and others added 2 commits June 25, 2024 13:57

Merge branch 'php:master' into stream_select_unlimited

2c146fa

fix: Added a cap to the number of file descriptors it can poll, in ca…

cad406e

…se an extremely high ulimit -n is configured.

frodeborli and others added 3 commits July 1, 2024 11:20

Merge branch 'php:master' into stream_select_unlimited

7e7ead3

fix: Updated stream_select implementation to dynamically resize the f…

9c3c9b5

…d_bigset structure whenever needed

fix: Unused argument max_fds removed from a couple of functions.

84a8e28

arnaud-lb reviewed Jul 2, 2024

View reviewed changes

Merge branch 'php:master' into stream_select_unlimited

8a15aa3

fix: Improvments suggested by @arnaud-lb

be6d295

bukka requested changes Jul 5, 2024

View reviewed changes

		@@ -756,115 +782,132 @@ static int stream_array_emulate_read_fd_set(zval *stream_array)
		/* }}} */


		if (!PHP_SAFE_MAX_FD(max_fd, max_set_count)) {
		if (max_set_count == 0) {

Updated the stream_select() function so that it supports bigger file … #14452

Are you sure you want to change the base?

Updated the stream_select() function so that it supports bigger file … #14452

Uh oh!

Conversation

frodeborli commented Jun 3, 2024

frodeborli commented Jun 3, 2024

frodeborli commented Jun 3, 2024

arnaud-lb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frodeborli commented Jun 3, 2024

devnexen commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ramsey commented Jun 4, 2024

Uh oh!

frodeborli commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

arnaud-lb commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

frodeborli commented Jun 5, 2024

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

frodeborli commented Jun 7, 2024 via email

Uh oh!

Uh oh!

frodeborli commented Jun 27, 2024

frodeborli commented Jun 27, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

frodeborli commented Jul 2, 2024

arnaud-lb commented Jul 3, 2024

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Jul 5, 2024

arnaud-lb left a comment •

edited

Loading

frodeborli commented Jun 4, 2024 •

edited

Loading

arnaud-lb commented Jun 4, 2024 •

edited

Loading

frodeborli commented Jun 27, 2024 via email •

edited

Loading