Skip to content

Fix cli server blocking on accept when using multiple workers #9693

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix cli server blocking on accept when using multiple workers #9693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion sapi/cli/php_cli_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2441,6 +2441,14 @@ static int php_cli_server_ctor(php_cli_server *server, const char *addr, const c
retval = FAILURE;
goto out;
}
// server_sock needs to be non-blocking when using multiple processes. Without it, the first process would
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be asserted by a test

Copy link
Member Author

@iluuu1994 iluuu1994 Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Theoretically we could observe the processes with strace, but the bug depends on a race-condition:

  1. OS receives the request
  2. Process 1/2 each
    a. select
    b. accept
    c. Handle other sockets

If 2.a. and 2.b. don't overlap the error is not triggered. A test would thus be unreliable. We barely have any tests for PHP_CLI_SERVER_WORKERS, some testing here is sensible for sure (I'll add that) but reliably testing this specific bug is probably difficult.

Copy link
Member Author

@iluuu1994 iluuu1994 Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm having trouble testing this on PHP-8.0, so I'll merge the test just into master. The good thing is that the test seems to catch the problem reported in this issue.

Edit: Sadly it does not.

// successfully accept the connection but the others would block, causing client sockets of the same select
// call not to be handled.
if (SUCCESS != php_set_sock_blocking(server_sock, 0)) {
php_cli_server_logf(PHP_CLI_SERVER_LOG_ERROR, "Failed to make server socket non-blocking");
retval = FAILURE;
goto out;
}
server->server_sock = server_sock;

php_cli_server_startup_workers();
Expand Down Expand Up @@ -2581,7 +2589,8 @@ static int php_cli_server_do_event_for_each_fd_callback(void *_params, php_socke
struct sockaddr *sa = pemalloc(server->socklen, 1);
client_sock = accept(server->server_sock, sa, &socklen);
if (!ZEND_VALID_SOCKET(client_sock)) {
if (php_cli_server_log_level >= PHP_CLI_SERVER_LOG_ERROR) {
int err = php_socket_errno();
if (err != SOCK_EAGAIN && php_cli_server_log_level >= PHP_CLI_SERVER_LOG_ERROR) {
char *errstr = php_socket_strerror(php_socket_errno(), NULL, 0);
php_cli_server_logf(PHP_CLI_SERVER_LOG_ERROR,
"Failed to accept a client (reason: %s)", errstr);
Expand Down