amphp / byte-stream

A non-blocking stream abstraction for PHP based on Amp.
https://amphp.org/byte-stream
MIT License
364 stars 31 forks source link

Stop using feof for checking if socket is closed #59

Closed psafarov closed 5 years ago

psafarov commented 5 years ago

Solves #58

bwoebi commented 5 years ago

Re-check the issue exposed by #40 before merging this one.

psafarov commented 5 years ago

From what I understood, feof tells if data was written to buffer, which happens to be an indication of a closed socket on some systems, however not on all of them, like in my case. So if fwrite is not good, then maybe fflush is. It flushes data from buffer and returns false on failure, which might be an actual indication of a broken connection.

trowski commented 5 years ago

I question calling fflush() after every write. Seems like it would be terrible for performance.

Based on the tests you changed, it looks like removing the EOF check results in multiple write attempts being made before the stream is detected as closed. Can the EOF check after the write be kept? I would expect this to return false. What about the is_resource() call?

psafarov commented 5 years ago

It does have to affect performance negatively, but I proposed it anyway because the feof check already prevented from buffer usage by throwing an exception, thus it is at least a lesser evil.

psafarov commented 5 years ago

And keeping EOF check after the write would not fix the problem. I ran a script as a systemd unit and feof was always true, either before or after the write.

psafarov commented 5 years ago

Resolved by #60