UniversalMediaServer icon indicating copy to clipboard operation
UniversalMediaServer copied to clipboard

Try to fix the resource leaks when streams are not closed properly

Open valib opened this issue 4 years ago • 4 comments

valib avatar May 30 '20 22:05 valib

@valib I've tried searching for answers about these kinds of changes and I am still confused. My concern is that returning the resource within a try-with-resources block will return a closed resource.

I found https://stackoverflow.com/questions/22947755/try-with-resources-and-return-statements-in-java and of course https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

It is probably best to design our code to return a result of a closeable stream, rather than the stream itself, but we have a lot of code that already does this pattern so maybe it is best for now to make sure the consumers of these methods which return autocloseable resources are closing them when they are finished using them. What do you think?

SubJunk avatar May 31 '20 02:05 SubJunk

You usually ignore my warnings, but I still can't not say anything about this one. It will completely break everything, try with resources will close the stream when it is exited. The Eclipse warnings aren't divine, there's a reason it's possible to suppress them.

Streams are often opened and consumed within the same method, and when that's the case, using try with resources is a nice way to protect against unforeseen events that break the normal logic so that the resource isn't closed, but it cannot be used if the stream is to ever leave the method.

For input streams it would theoretically be possible to consume them locally, close the stream and pass on the content, but that requires reading the entire resource into memory. This isn't always a good idea, especially when we're dealing with media files. For output streams it's simply impossible to get around this unless you already have all the data you with to write at the time you open the stream.

In short, in more "complex" situations where you work with a stream over time or using multiple methods, you can't use try with resources. You must instead be vigorous and making sure that there are no paths, regardless of how unlikely, where the resource isn't closed - including when exceptions are thrown at any location where that is possible. If you fail to do that, you have a leak...

Nadahar avatar May 31 '20 14:05 Nadahar

@SubJunk @Nadahar if the try-with-resources returns closed resource or not it depends on the Java implementation. E.g. the InputStream.close() called by the try-with-resources to close does nothink see the comment in the Java source

/** * Closes this input stream and releases any system resources associated * with the stream. *

The close method of InputStream does * nothing. * * @exception IOException if an I/O error occurs. */ public void close() throws IOException {}

Maybe that is why it works for me. Anyway I rewrite some changes.

valib avatar May 31 '20 22:05 valib

@valib Yes, it's true that not all implementations implements close(), but most do. InputStream is an abstract class, so methods are often overridden in implementations. File-based streams will certainly fail if you try to access them after closure, but memory-based streams probably don't care (like ByteArrayInputStream for example).

Nadahar avatar May 31 '20 22:05 Nadahar

This got stale, closing

SubJunk avatar Mar 03 '23 00:03 SubJunk