Fixes #68948 related to a BC break introduced by #68532 fix.

[jails]

This PR is an alternative to #1153 which is a attempt to address #68948 and #68481 without breaking #68532.

Right from start feof() on memory based streams didn't work the same way as on file based streams (although it works as expected on HHVM).

The following PR changed the memory based streams behavior to solve an eof realted issue. However despite the BC Break, the new introduced behavior was still not consistent with file based streams behavior. And unfortunately there's no way to do so.

Indeed if the current behavior of file based streams is correct, the issue comes from the extrapolation of the eof state from read(). Indeed eof can be safely set to 1 only when read() returns 0.

So to make it works, this logic requires an extra read which is satisfied by the following condition. Indeed if the number of bytes actually read < size, the loop is executed once more. read() will return 0, eof will be set to 1 and we are done

But it's not possible to make other streams to work the same way because of this specific check. However due to the current eof logic this break is at least necessary for blocking streams (indeed an extra read will hang the whole process).

So either php_stdiop_read can rely on fread() and the whole _php_stream_read && _php_stream_fill_read_buffer will need to be refactored according to feof() behavior.

Or we can switch to greedy reads for all non blocking streams like in the following PR to the make the stream API consistent.

PS:
I noticed that stream->wrapper was null for some socket based streams but I have no idea if this "trick" can work for all other blocking streams.

[jails]

Since the 7.0.0 is now out, I'm assuming this issue wasn't considered as major. So now it has been released what can I do to help further and having this old issue fixed ?

[krakjoe]

What a mess :)

It's difficult for anyone to move on this while the PR isn't generating interest or discussion, what we need is people intimately familiar with streams to come and give us their opinion.

I realise it's very late in the day, sorry about that ...

What I suggest now is that you start an internals discussion to try to attract some attention for these issues, this should draw out some commentators, and build some kind of consensus on this issue.

[jails]

Thank you for the head up @krakjoe I will do that 👍

[nikic]

Also ping @sgolemon

[krakjoe]

@jails can we know the current status/plan here please ?

If you started an internals discussion can you link back to it here.

[jails]

@krakjoe sorry I completly forgot about that, just subscribed, validated my email and sent a message explaining the situation to the php.internals mailing list.

[jails]

I threw the bottle into the sea, so wait and see.

[sgolemon]

Indeed eof can be safely set to 1 only when read() returns 0.

The end result of your memory.c change appears to be exactly the same except for the edge case of fread($memoryStream, 0) which is an odd duck so nevermind it for now.
The existing code checks if pos == size and sets eof based on that. This is, in effect, a read of the memory stream returning zero.

The reason your tests start passing with this patch seems to do a lot more with the change to streams.c, the intent of which I'm fine with, but the logic needs work. Perhaps (and I haven't thought this through all the way yet) something like: if (!stream->wrapper || stream->wrapper->is_url) { ... } . "is_url" is unfortunately named, since they're all URLs really. It means, "This is a remote (e.g. socket based) resource."

PS:
I noticed that stream->wrapper was null for some socket based streams but I have no idea if this "trick" can work for all other blocking streams.

Uh.... that's probably a bug. All open streams should have wrappers. :/ Can you separately create a repro script for that?

[jails]

Thanks @sgolemon for the review ! You are totally right, changes on memory.c was indeed superfluous, I don't remember why I added it in the first place.
And streams have wrappers (I wasn't able to reproduce the issue I was talking about so probably a mistake of my part).
So I updated the PR according your comment and using the condition if (!stream->wrapper || stream->wrapper->is_url) { ... } was enough to make all tests passing !
Is there anything I can do to help further here ?

[krakjoe]

@sgolemon could you take another look here please ?

[jails]

@krakjoe since all tests are passing maybe we can ship this in the current 7.2 alpha release and see how it's going ?

[krakjoe]

@sgolemon @remicollet could you take a look at this one please ?

[Tyrael]

any update on this?

[jails]

If nobody is comfortable with this part of the codebase maybe the best option is simply to merge this fix so we can move on. This bug worth any other bug and we can't get stuck on it forever imo.

[sgolemon]

This is my fault for not paying attention to my github notifications.
I'm happy with this change, my only hesitation is that we're now brushing up on RC6 for 7.2.0 (due to my lack of attention).

Yeah, eff it, merging... On my head be it.

[php-pulls]

Comment on behalf of pollita at php.net:

This will be on 7.2.0RC6