Skip to content

Implement GH-23879: Do not report an error on a zero accept timeout - #23880

Open
lazerg wants to merge 4 commits into
php:masterfrom
lazerg:fix/gh-23879
Open

lazerg wants to merge 4 commits into
php:masterfrom
lazerg:fix/gh-23879

Conversation

@lazerg

@lazerg lazerg commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

When stream_socket_accept() timed out with no client waiting, it went down the same path as a real failure and raised AcceptFailed ("Connection timed out"). That makes the usual "accept until empty" drain loop with a 0 timeout throw in StreamErrorMode::Exception, and the only way to tell a timeout apart from something like fd exhaustion was the message text.

The TCP and TLS transports now treat a poll timeout or EAGAIN as "nothing to accept" and return success without a client. stream_socket_accept() then returns false with no error for a 0 timeout, and reports TimeOut instead of AcceptFailed when a non-zero timeout expires. Real accept failures still report AcceptFailed. PHP_TIMEOUT_ERROR_VALUE moved from network.c to php_network.h so the transports can check for it.

Closes #23879

@bukka bukka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I just noted in #23879, this is not a bug. So it should target master.

I'm also not sure why it does it only for non-blocking. It shouldn't really matter that much. Also there is a bit semantic change in php_stream_xport_accept() return value so that should probably be noted in UPGRADING.INTERNALS (so it couldn't probably target 8.6 in any case).

@lazerg lazerg changed the title Fix GH-23879: Do not report AcceptFailed on non-blocking accept timeout Implement GH-23879: Do not report AcceptFailed on accept timeout Oct 4, 2026
@lazerg
lazerg changed the base branch from PHP-8.6 to master October 4, 2026 18:31
@lazerg

lazerg commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

I rebased onto master and dropped the non-blocking check in 5178927, so a timeout with no pending connection is now silent on blocking listeners too. The UPGRADING.INTERNALS note for php_stream_xport_accept() was already in the PR. I moved it to the 8.7 section and added an UPGRADING entry for the dropped error.

@bukka

bukka commented Oct 4, 2026

Copy link
Copy Markdown
Member

Hmm I think it should still produce error if it's not a 0 timeout. It might make sense to change that error to TimeOut . Only the 0 timeout would be ignored then.

@lazerg lazerg changed the title Implement GH-23879: Do not report AcceptFailed on accept timeout Implement GH-23879: Do not report an error on a zero accept timeout Oct 5, 2026
@lazerg

lazerg commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

I changed it in 1cd4470. A 0 timeout with no pending connection is now silent. When a non-zero timeout expires, stream_socket_accept() reports TimeOut instead of AcceptFailed. The transports still return 0 with no client on a timeout, so stream_socket_accept() picks the code from the timeout it passed. The message text did not change.

@TimWolla

TimWolla commented Oct 5, 2026

Copy link
Copy Markdown
Member

As I just noted in #23879, this is not a bug. So it should target master.

As I replied there, I disagree on that and believe it makes a new feature pretty much unusable in practice.


It looks like this PR can resolve the issue without introducing any API / ABI change, making it theoretically applicable to PHP 8.6. I've requested PHP 8.6 RM review for them to decide.

@TimWolla
TimWolla requested a review from a team October 5, 2026 08:23
@bukka

bukka commented Oct 5, 2026

Copy link
Copy Markdown
Member

I think we need to first agree whether it is a bug or feature. RM cannot decide this and we require full agreement on such topic. Currently we don't have such agreement - let's keep that discussion in the actual issue so we don't duplicate it here.

But even if we agreed that this this is a bug, there is, however, still semantic internal API change that this would introduce. It changes the php_stream_xport_accept() contract to return 0 with a NULL client and it currently changes the reported code for non zero timeouts

@bukka
bukka removed the request for review from a team October 5, 2026 09:15
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.

Do not report AcceptFailed when timing out in stream_socket_accept() for non-blocking listeners

3 participants