resource leak: audio manager

[maggu2810]

If my understanding of the audio managers playFile implementation is correct then there is a resource leak.
At least for me it seems that the input stream if not closed reliable (if at all).

[maggu2810]

@kaikreuzer Should we mark resource leaks with a "critical" label?

[kaikreuzer]

It depends whether we consider it critical ;-)
I'd define critical to be:

• makes the system dysfunctional for a large number of users under normal operation
• vulnerabilities

I am personally using playFile on a daily basis and haven't seen any negative impact (system running for months without an issue). So I'd not really consider it to be critical unless I miss something.

[maggu2810]

For me it is different.
For me resource leaks are critical because one component can eat all resources that are not available for other components anymore.
Just because nobody runs into it doesn't make it less critical.

If a productive system fails because it runs out of resources, it could be hard to find the root cause (network traffic, files, ...).

In the special case the input stream of an FileAudioStream is never closed.
Perhaps also my reading of the sources has been wrong and the problem itself does not exist.

On my working machine it fails on the "1023268" try to open an file input stream without closing it.
If we know that the usage of FileAudioStream is the only resource leak, perhaps you will not hit that number ever...

... but openHAB Core as a framework should not matter about its usage and do its best to be flexible and "safe" (what a magic word) to use.

So, okay if you don't want to mark that one as critical -- for me it stays on the list of critical issues (as soon as I start to use the audio support). 😉

[cweitkamp]

I think it is not handled correctly anymore. But currently we do not face a resource leak because of two things.

• Some of the AudioSink implementations in bindings use IOUtils.closeQuietly() in the process() method to handle the stream (see for example here) or close them on their own. That is obviously not what a framework expect the outside world to do.

• The AudioServlet implementation closes the streams via IOUtils once they will be expired (see https://github.com/openhab/openhab-core/pull/801/files#r281794201).

Latest Apache Commons IO 2.6 API removed the IOUtlis.closeQuietly() methods completely in favor of the try-with-resources statement. From my point of view we should follow their path and get rid of it too.

Do you think we should /could update Apache Commons IO dependency too? We include version 2.2.0. What are the plans for it?

[maggu2810]

There are other things I don't understand. Perhaps you can give me some more information:

The AudioServlet provides two methods to "serve" an audio stream. One with a timeout and one without a timeout. If I call the "serve" method, who is responsible for calling close() of the stream?
Is the ownership of the stream given to the AudioServlet implementation and should this one close the stream or is the caller in any way still responsible to close the stream.
We should document the intentional behaviour on that methods.
@kaikreuzer IIRC this stuff has been written by you. Can you give me the information?

The current implementation of the AudioServlet stores the streams and close it on some situation.
If the service gets deactivated NO stream is closed. So, if the caller is responsible to close the streams, it is no problem. But I don't think the one that wants to serve a one time stream can close the stream as the caller does not know at which time the stream has been served once.

[maggu2810]

@kaikreuzer friendly ping

[kaikreuzer]

If I call the "serve" method, who is responsible for calling close() of the stream?

This is the AudioServlet as only this knows when a stream has been requested and delivered and when it can be closed.

If the service gets deactivated NO stream is closed.

This is then clearly a bug that should be fixed.