Skip to content

Return immediately when FD_SETSIZE is exceeded #9602

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

Merged
merged 3 commits into from
Oct 1, 2022

Conversation

arnaud-lb
Copy link
Member

Fixes #9590

@arnaud-lb arnaud-lb marked this pull request as ready for review September 24, 2022 13:28
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just few NITS but otherwise all looks reasonable to me.

Comment on lines +23 to +53
posix_setrlimit(POSIX_RLIMIT_NOFILE, 2048, -1);

$fds = [];
for ($i = 0; $i < 1023; $i++) {
$fds[] = @fopen(__DIR__ . "/GH-9590-tmpfile.$i", 'w');
}

require_once('connect.inc');

function get_connection() {
global $host, $user, $passwd, $db, $port, $socket;

if (!$link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket))
printf("[001] [%d] %s\n", mysqli_connect_errno(), mysqli_connect_error());
return $link;
}


$mysqli1 = get_connection();
$mysqli2 = get_connection();

var_dump(mysqli_query($mysqli1, "SELECT SLEEP(0.10)", MYSQLI_ASYNC | MYSQLI_USE_RESULT));
var_dump(mysqli_query($mysqli2, "SELECT SLEEP(0.20)", MYSQLI_ASYNC | MYSQLI_USE_RESULT));

$links = $errors = $reject = array($mysqli1, $mysqli2);
var_dump(mysqli_poll($links, $errors, $reject, 0, 50000));

mysqli_close($mysqli1);
mysqli_close($mysqli2);

print "done!";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: extra indent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many tests in this directory are like that, I tried to keep the existing style

@bwoebi
Copy link
Member

bwoebi commented Sep 25, 2022

I guess just always returning is the easiest thing to do :-)

I'm not sure whether it's better to generally allow graceful handling or just return, but I'm fine with this too.

arnaud-lb and others added 2 commits September 26, 2022 14:15
Co-authored-by: Jakub Zelenka <bukka@php.net>
Co-authored-by: Jakub Zelenka <bukka@php.net>
@arnaud-lb
Copy link
Member Author

@bwoebi I'm open to suggestions. Do you have a solution in mind to handle this gracefully ? Currently this change causes select() to return false when FD_SETSIZE is exceeded instead of hanging forever.

@bwoebi
Copy link
Member

bwoebi commented Sep 26, 2022

The other option is to just return when an exception is thrown or when the fd set is empty.
I.e. mirroring current behaviour, except for the specific reported edge cases.

But I'm not sure whether that's better.

@arnaud-lb
Copy link
Member Author

I have a preference for just returning when an exception is thrown, or for always returning when FD_SETSIZE is exceeded, over returning only when the fd set is empty. In the latter, the user may be under the impression that select() actually works despite the warning, because select() will monitor some FDs, but some others will not actually be monitored.

Returning only when an exception is thrown has the advantage of making the least behavioral changes while still fixing the original bug.

Returning when FD_SETSIZE also prevents hanging forever (in the case where the set is empty), and makes the problem more visible (if the warning is not spotted already).

@bwoebi
Copy link
Member

bwoebi commented Sep 30, 2022

Fine by me :-)

@arnaud-lb arnaud-lb merged commit 80232de into php:PHP-8.0 Oct 1, 2022
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Oct 1, 2022
* PHP-8.0:
  [ci skip] NEWS
  Return immediately when FD_SETSIZE is exceeded (php#9602)
arnaud-lb added a commit that referenced this pull request Oct 1, 2022
* PHP-8.1:
  [ci skip] NEWS
  [ci skip] NEWS
  Return immediately when FD_SETSIZE is exceeded (#9602)
arnaud-lb added a commit that referenced this pull request Oct 1, 2022
* PHP-8.2:
  [ci skip] NEWS
  [ci skip] NEWS
  [ci skip] NEWS
  Return immediately when FD_SETSIZE is exceeded (#9602)
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.

stream_select does not abort upon exception or empty valid fd set
4 participants