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

Some classes that perform Input/Output (LengthOf, BytesOf, ...) close the stream #1449

Closed
fabriciofx opened this issue Sep 10, 2020 · 25 comments

Comments

@fabriciofx
Copy link
Contributor

Some classes as LengthOf closes the Input after performed the operation. But often we need keep the Input stream opened even after it. So, let's create a decorator to Input and Output that allows keep these streams opened.

@victornoel
Copy link
Collaborator

victornoel commented Sep 10, 2020

@fabriciofx or we could also just not close those Input or Output in any class and introduce some concept to wrap operations that manipulates Input/Output to close them at the end (a bit like try-with-resource).

I'm not sure what is best…

edit: see #1449 (comment)

@fabriciofx
Copy link
Contributor Author

@victornoel I think not close after the manipulation better. As say the golden rule: *"Who allocated the resource is the responsible to deallocated it". A try-with-resourced kind of class is a good idea too. :)

@fabriciofx fabriciofx changed the title NotClose decorator to Input/Output Some classes that perform Input/Output (LengthOf, BytesOf, ...) close the stream Sep 10, 2020
@victornoel
Copy link
Collaborator

@fabriciofx actually, after some thought, I'm not sure, because when you call Input.stream(), it means you could retrieve a new stream, so you HAVE to close it at the end.

Then, it means your first proposal was actually best indeed: i.e., it is the Input object creator that decides how the streams returned from stream will behave.

@victornoel
Copy link
Collaborator

@0crat in

@victornoel
Copy link
Collaborator

@fabriciofx some extra requirements for this job for the DEV that will take it: we must find all the classes that closes the stream like this, document them about this behaviour and refer to the existence of the new classes we should introduce that prevent stream closing

@victornoel victornoel added this to the 1.0 milestone Jan 16, 2021
@baudoliver7
Copy link
Contributor

baudoliver7 commented Mar 4, 2021

@victornoel Let's me find first all the classes that close the stream like mentioned by @fabriciofx, document them properly and add a todo. Isn't it ?

@victornoel
Copy link
Collaborator

@baudoliver7 you can do it either order you want yes: first document then todo for implementing the new decorators, or first implement the new decorators and then todo to document the classes.

@baudoliver7
Copy link
Contributor

@victornoel Ok. Let's us implement the new decorators :)

@baudoliver7
Copy link
Contributor

@victornoel What names could you suggest me ?

@victornoel
Copy link
Collaborator

@baudoliver7 hehe, no idea actually, @fabriciofx would you have an idea?

Could you propose a name and we discuss it in the PR maybe?

@baudoliver7
Copy link
Contributor

@victornoel Yes, I will do it.

@andreoss
Copy link
Contributor

andreoss commented Mar 8, 2021

@fabriciofx
Keeping OutputStream opened might be handy, but what is the point of doing so for InputStream?

First invocation of new LenghtOf(new SafeInput(input)) will return correct length, the second will return 0 in your case, instead of throwing an exception (current behaviour). SafeInput should have some mark/reset logic at close for the original stream in order to avoid that.

Also LengthOf sometimes is explicitly used as a terminal operation on streams (with TeeInput for example), so if LengthOf won't close its streams existing code will break

baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 8, 2021
@baudoliver7
Copy link
Contributor

baudoliver7 commented Mar 8, 2021

@andreoss
You are right. The first invocation of new LenghtOf(new SafeInput(input)) will return correct length, the second will return 0 in our case. But we fixed it in the last commit. Thanks.

@baudoliver7
Copy link
Contributor

@andreoss Also, if someone want LengthOf to close original streams, he should not have to decorate original Input with SafeInput. But in this case, the class that allocated the resource is responsible to deallocated it.

@victornoel
Copy link
Collaborator

@andreoss @baudoliver7 it is normal that using LengthOf twice on a SafeInput will have the second time return 0, we don't want to prevent this. And actually, you don't know what the underlying stream is made of: maybe you read it once, then later on, there is again some data in it. That's why you need something like SafeInput, to prevent the stream consumer to close it when, as the user (or maybe creator) of that stream, you do know its behaviour is a bit more tricky than that.

This also applies to ouput: when I am producing a Zip with ZipOutputStream, I dont want to close the stream inbetween writing each file for example.

The whole point is to give more power to the code that composes the various objects together, because that code may know better than the code consuming the stream.

baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 8, 2021
baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 13, 2021
baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 14, 2021
baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 14, 2021
baudoliver7 pushed a commit to baudoliver7/cactoos that referenced this issue Mar 14, 2021
@rultor rultor closed this as completed in 81a405f Mar 22, 2021
@0pdd
Copy link
Collaborator

0pdd commented Mar 22, 2021

@fabriciofx the puzzle #1577 is still not solved.

@victornoel
Copy link
Collaborator

victornoel commented Mar 22, 2021

@fabriciofx sorry, this was automatically closed because the "closed" keyword was in the commit message ^^ I hope the solution is good for you, if not please reopen a ticket :)

@0crat
Copy link
Collaborator

0crat commented Mar 22, 2021

@rultor/z the issue is closed not by @fabriciofx/z (its creator); I won't close the order; please, re-open it and ask @fabriciofx/z to close it

@victornoel
Copy link
Collaborator

@0crat out

@0crat 0crat added the qa label Mar 22, 2021
@0crat
Copy link
Collaborator

0crat commented Mar 22, 2021

@sereshqua/z please review this job completed by @baudoliver7/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@0crat
Copy link
Collaborator

0crat commented Mar 22, 2021

@0crat out (here)

@victornoel Job gh:yegor256/cactoos#1449 is not assigned, can't get performer

@sereshqua
Copy link

@0crat quality good

@0crat 0crat removed the 0crat/scope label Mar 22, 2021
@baudoliver7
Copy link
Contributor

@0crat status

@0crat 0crat added quality/good and removed qa labels Apr 7, 2021
@0crat
Copy link
Collaborator

0crat commented Apr 7, 2021

@0crat status (here)

@baudoliver7 This is what I know about this job in C63314D6Z, as in §32:

@0pdd
Copy link
Collaborator

0pdd commented Jul 11, 2022

@fabriciofx the puzzle #unknown is still not solved.

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

No branches or pull requests

7 participants