Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Apr 28, 2021

Just a PoC to check for obvious issues.

@nikic
Copy link
Member

nikic commented Apr 28, 2021

With this change a) is it possible for select to report a readable stream, but reading it will read zero bytes (because filter is waiting for data)? b) if yes, does that violate the select contract (does it allow "spurious wake"?)

@cmb69
Copy link
Member Author

cmb69 commented Apr 28, 2021

Oh, that can happen indeed. So we cannot cast filtered streams to PHP_STREAM_AS_FD_FOR_SELECT.

@cmb69 cmb69 closed this Apr 28, 2021
@nikic
Copy link
Member

nikic commented Apr 28, 2021

Well, that depends on what the answer to the second question is ^^ Maybe it's okay...

@cmb69
Copy link
Member Author

cmb69 commented Apr 29, 2021

if yes, does that violate the select contract (does it allow "spurious wake"?)

I don't know, and failed to find any relevant info about that. We're usually (in practise always?) calling recv(2) with MSG_DONTWAIT, so that may not cause spurious wakes, but we also have a fallback definition (https://github.com/php/php-src/blob/php-7.4.18/main/streams/xp_socket.c#L32-L34), so …

@bukka
Copy link
Member

bukka commented Dec 16, 2023

I have been looking into this and think the mentioned "spurious wake" can already happen for TLS if there is less than the TLS record size read. I might need to verify it but don't think it's an issue. The trickier part is to find out if anything is buffered in the filters and "wake" should happen even though nothing ready to read from fd. I have a feeling that solution for that would be probably for master only but will check that more.

@bukka
Copy link
Member

bukka commented Nov 20, 2025

For the reference I just created a PR for this with tests in #20540 . I did further investigation and I think it's safe (at least for master which I'm targeting).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants