Hi,
this PR closes #28221 by using InputStream.transferTo where possible.
I'm not super convinced of that PR to be honest because for consistency reasons and to make sure it works as before, we need to call flush on the OutputStream - which makes it a bit less convenient to be honest (two lines instead of one). While this wouldn't be necessary for FileOutputStream strictly speaking, because its implementation does nothing, I added it everywhere.
Let me know what you think. Cheers, Christoph
Comment From: mdeinum
Wouldn't it make more sense to modify the StreamUtils.copy method instead? And update the BUFFER_SIZE to the same default as used inside the JDK (which is 8192 double of what StreamUtils is using)?
Comment From: dreis2211
Certainly an option @mdeinum, but if there is a "native" equivalent why use the utility. The question is only if we are okay with the additional flush or not. There has been a PR at https://github.com/spring-projects/spring-framework/pull/27702/files that talked about it. As things stand the PR isn't doing this anymore though.
I don't have strong feelings for either option, but it's easier to argue about it when you see the code - hence the PR :)
Comment From: philwebb
Having looked at the changes I'm not sure we should proceed. I think when I raised #28221 I thought that it would be a 1-1 replacement, with the possibility that Framework might deprecate their version. The flush method makes things a bit more involved and I think I prefer keeping things as they are.
Flagging to see what others think.
Comment From: scottfrederick
I agree with Phil, the .flush() calls with every .transferTo() is a bad smell, and looks more cluttered than just using StreamUtils.copy().
Comment From: philwebb
I think I'm going to close this one. Sorry about the wasted effort @dreis2211 :(
Comment From: dreis2211
@philwebb It wasn't wasted effort - the point was to demonstrate how it looks and since we can't do video calls that was the easiest way. Don't worry :)