Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResourceOutputStream interrupts writing without consumer giving information what was written/unwritten #51

Closed
ostrolucky opened this issue Feb 25, 2019 · 11 comments

Comments

@ostrolucky
Copy link
Contributor

ostrolucky commented Feb 25, 2019

I'm dealing with situation that following line

throw new StreamException($message);
throws exception, but doesn't expose information what was written/what is left to write, which makes it very difficult to handle such situation in case of incomplete chunk write. My aim is handling gracefully such exception and re-writing this data to different stream.

I suggest to create custom exception which carries $data it was not succeeded to write.

@kelunik
Copy link
Member

kelunik commented Feb 25, 2019

This won't really help, since you still won't know which data the other and received. Data may have been written to the kernel's buffer but been sent.

@ostrolucky
Copy link
Contributor Author

It will help, I tested that. Just expose what information that class already has.

@kelunik kelunik closed this as completed Feb 25, 2019
@ostrolucky
Copy link
Contributor Author

ostrolucky commented Feb 25, 2019

Why was this closed without explanation?

ResourceOutputStream checks if length of $data equals to value from fwrite. If it doesn't match, it repeats the write with unwritten data. If it doesn't succeed, it throws exception. I need to know what it tried to write last time.

@kelunik kelunik reopened this Feb 25, 2019
@kelunik
Copy link
Member

kelunik commented Feb 25, 2019

I didn't intend to close this, yet. Probably mistyped on my phone.

@trowski
Copy link
Member

trowski commented Feb 25, 2019

As @kelunik pointed out, how useful is that information? When you write to a socket, you are only receiving confirmation that the OS has room in it's buffer for the data written. There is no guarantee that the client actually received that data.

@ostrolucky
Copy link
Contributor Author

ostrolucky commented Feb 26, 2019

That's all I need. If I don't send this data at all, it's definitely lost. If I do, there is high chance of this data reaching destination. Also not sure why are you talking about socket again, that's not the only resource. Do you have the same viewpoint for eg. files? fwrite returning > 0 not being guarantee for data being written to file? That seems kinda ridiculous to me. But once again, I don't understand how is not having guarantee of data reaching destination excuse for not handling known failure when sending it in the first place.

@kelunik
Copy link
Member

kelunik commented Mar 11, 2019

Re-opened, as it has been closed by the merge, but the merged commit has been reverted.

@kelunik
Copy link
Member

kelunik commented Mar 11, 2019

@ostrolucky ResourceOutputStream shouldn't be used for files, it just happens to work.

I'm not 100% opposed to exposing the information, but I think it is misleading and will not report what most people expect. I'd be cool to have information from the operating system which data has been ACKed by the receiving party, but we currently don't have that data.

@kelunik
Copy link
Member

kelunik commented Mar 11, 2019

I think it's a better idea to expose the unwritten bytes instead of the written chunk if we choose to expose that information, as with that information you can directly attempt a new write on reconnection or whatever your logic is.

@ostrolucky
Copy link
Contributor Author

ostrolucky commented Mar 12, 2019

That patch reported data that didn't succeed to be written. Turning it to int is more complicated from both sides - library and consumer. I need that data so I can repeat write.

@kelunik
Copy link
Member

kelunik commented Aug 23, 2019

I'm closing this in favor of amphp/file#35. We don't want to expose this information for every stream in this library, because for sockets it's generally misleading.

@kelunik kelunik closed this as completed Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants
@ostrolucky @trowski @kelunik and others