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

Throw new exception carrying $chunk when fwrite returns 0 #52

Closed ostrolucky closed 5 years ago

ostrolucky commented 5 years ago

Fixes #51

I tried hard to write a test for simulating such write failure, but any trick I tried, stream_select will not work with those. Another option would be to use unqualified fwrite call, but I don't think that would be approved.

ostrolucky commented 5 years ago

friendly ping @kelunik @trowski I need this

bwoebi commented 5 years ago

I think this can be tested with a custom stream wrapper - I'll have to check. But the change itself is fine - thanks :-)

ostrolucky commented 5 years ago

Nope I tried that trick, that's how I discovered this https://github.com/amphp/amp/issues/265

edit: btw thanks for merge (。◕‿◕。)

kelunik commented 5 years ago

I'm sorry, but I've reverted the commit. The reported information is incorrect in case write-splitting is used, see https://github.com/amphp/byte-stream/blob/2120bdd46b2c9602025f0e826971beb515eaab8f/lib/ResourceOutputStream.php#L208-L215.

Additionally, I still think giving out that information is misleading for the intended use case, because written bytes mostly don't let you know what the other side received. Those cases where that is the case, are not intended to use ResourceOutputStream AFAIK.

ostrolucky commented 5 years ago

I don't see how is information incorrect in case write splitting is used. It reports what part of chunk wasn't successful to be written. It doesn't matter that original chunk was splitted, it still returns correct information what data failed to be written

kelunik commented 5 years ago

@ostrolucky It will only return a part of the original chunk, not data based on the complete chunk.

ostrolucky commented 5 years ago

Yes, that's what I need. I need data that failed to be written, which might be just part of original chunk. I already have original chunk when passing it to write($chunk). What I don't have is information which part of chunk failed to be written because of internal splitting and retry logic.

I don't understand second part of your sentence though. Splitting is by definition based on complete chunk.

ostrolucky commented 5 years ago

Maybe $chunk confused you? It could use better naming. It's not original chunk, but last $data passed to fwrite

trowski commented 5 years ago

@ostrolucky When the chunk is split, the implementation was only returning one portion of the entire split chunk. Also any other chunks in the queue were not included. All remaining chunks in the write queue should be concatenated together if you want the entirety of what was not written.

As I said in the linked issue, I don't think this information is useful. It's misleading at best and potentially dangerous. A chunk reported as written is not at all guaranteed to have been received at the other end. I can set up a stream between two machines on my local network, unplug the receiver, and at least a few more chunks never received are reported as written. Additionally, attaching a raw byte-stream to an error can have security implications.

I don't exactly know what your use-case is, but I'm going to guess it's serving files over HTTP. Look into byte-range requests. The client should be telling you what bytes they need rather than guessing.

ostrolucky commented 5 years ago

A chunk reported as written is not at all guaranteed to have been received at the other end.

But chunk reported as unwritten is guaranteed to have not been received at the other end. This is what is this about. I don't care if client recieved all data, I care if all was sent. I can't do anything about data it cannot receive.

Ok I will see what can be done about extra chunk splitting.

trowski commented 5 years ago

How is that information useful though?

ostrolucky commented 5 years ago

This is prerequisite to do failure recovery. I want to write data failed to be written in one stream into another stream. What I must NOT have is overlapping data in one or other stream.

kelunik commented 5 years ago

@ostrolucky Which data format are you using? Most data formats aren't happy with arbitrary missing chunks in the middle.

ostrolucky commented 5 years ago

Binary