Skip to content

Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.#221

Merged
garydgregory merged 1 commit intoapache:masterfrom
arturobernalg:feature/newOutputStream
Jun 30, 2021
Merged

Replace construction of FileInputStream and FileOutputStream objects with Files NIO APIs.#221
garydgregory merged 1 commit intoapache:masterfrom
arturobernalg:feature/newOutputStream

Conversation

@arturobernalg
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Apr 9, 2021

Coverage Status

Coverage remained the same at 89.265% when pulling 245cb08 on arturobernalg:feature/newOutputStream into 6514969 on apache:master.

@garydgregory
Copy link
Member

@arturobernalg
May you please rebase on master which should allow the Java 17-EA build to pass.

@arturobernalg
Copy link
Member Author

@arturobernalg
May you please rebase on master which should allow the Java 17-EA build to pass.

HI @garydgregory
Done.
TY

throw new IOException("Cannot create file " + testFile + " as the parent directory does not exist");
}
final BufferedOutputStream output = new BufferedOutputStream(new FileOutputStream(testFile));
final BufferedOutputStream output = new BufferedOutputStream(Files.newOutputStream(testFile.toPath()));
Copy link
Member

@garydgregory garydgregory May 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these test methods, you keep calling toPath() over and over, why not just call it once? IOW should testFile be testPath or have both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @garydgregory
you're right. Changed.
TY

@jochenw
Copy link
Contributor

jochenw commented May 23, 2021

I am unsure about this one. First of all, it changes way too many files, in my opinion.
Then, I see no real benefit, when comparing Files.newInputstream() to new FileInputStream(). As far as I know, there is no real difference between those streams. In particular, neither is buffered.

@arturobernalg
Copy link
Member Author

Files.newInputstream() to new FileInputStream()

HI @jochenw
This is the reason -->
'Basically, FileInputStream and FileOutputStream both use a finalizer to ensure that the resources are closed.
Even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).'

@garydgregory
Copy link
Member

Files.newInputstream() to new FileInputStream()

HI @jochenw
This is the reason -->
'Basically, FileInputStream and FileOutputStream both use a finalizer to ensure that the resources are closed.
Even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).'

Indeed, @jochenw do notice java.io.FileInputStream.finalize() and java.io.FileOutputStream.finalize(). I'll review further but I am incline to merged based on the finalize() issue. While in the long term I think finalization is going away, we are still on Java 8 for now.

@garydgregory garydgregory merged commit b2811e8 into apache:master Jun 30, 2021
asfgit pushed a commit that referenced this pull request Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants