reactphp / socket

Async, streaming plaintext TCP/IP and secure TLS socket server and client connections for ReactPHP.
https://reactphp.org/socket/
MIT License
1.21k stars 157 forks source link

Fix possible `accept()` race condition when multiple socket servers listen on same socket address #244

Closed clue closed 4 years ago

clue commented 4 years ago

Multiple socket servers can listen on the same socket address (using SO_REUSEPORT or shared file descriptors). Accordingly, when a new connection is incoming, multiple event-loops could report the socket to be readable and try to a accept() an incoming connection from the same socket.

This wouldn't be an issue with the underlying accept() system call. The first call would succeed and subsequent calls would report EAGAIN or EWOULDBLOCK for the other servers given the socket resource is in non-blocking mode.

However, PHP's stream_socket_accept() implementation first runs a poll() on the socket resource before performing an accept(). This means multiple instances listening on the same address could end up in a race condition where some may be "stuck" in the pending poll(). See https://github.com/php/php-src/blob/91fbd12d5736b3cc9fc6bc2545e877dd65be1f6c/main/network.c#L707

We work around this by specifiying a 0 timeout to ensure the poll() doesn't block in this case. This allows all servers to either complete successfully or report an error and continue processing right away.

Additionally, this also fixes two test failures for PHP 8. Instead of using an invalid (closed) socket server to test error reporting, we now use a socket that doesn't have a pending connection (refs #243). I'll file another PR address the last test failures on PHP 8, but this is currently blocked by #243.

Among others, this is a prerequisite for #164