Possible resource leak in OnSubscribeUsing

[dimas]

Hi.
I am considering using of RxJava to represent REST API responses so was looking into the sources to understand how stuff works. In the OnSubscribeUsing operator there is something that looks like a logic error to me that may lead to leaking resources.

(I am reporting "theoretical" issue, I have not tried actually making it happen. Also, I have almost zero knowledge on how RxJava works so can be really missing something. Sorry about that in advance)

So in https://github.com/ReactiveX/RxJava/blob/1.x/src/main/java/rx/internal/operators/OnSubscribeUsing.java the call() method allocates resource the first thing and then creates Observable, attaches dispose handler etc. In the end it calls unsafeSubscribe handling for the potential exception.
But what if exception happens earlier? Imagine the resource was allocated successfully but observableFactory.call(resource) throws for example. There seems to be nothing to dispose the resource in that case.

Again. I have no deep understanding of RxJava but it looks to me that resource disposal should be happening in the top level try/finally block if resource was allocated but doOnTerminate has not been installed yet (or failed to install). I would probably just remove the inner try/catch and did all the handling at the top level (possibly setting some boolean to indicate that stuff actually started so no cleanup needed).

Cheers

[akarnokd]

Thanks for reporting! This is indeed a bug and I've posted a fix PR #3922 for it.

[dimas]

Oh that was quick. Impressive )
Just out of curiosity - is it kind of implied that thing like

 subscriber.add(disposeOnceOnly);

always succeed and cannot throw? As well as doOnTerminate / doAfterTerminate.
Isn't it safer to always do dispose in a big finally block unless unsafeSubscribe was successful? That would probably also simplify exception handling as now you have two identical copy&paste blocks...

Sorry if I am asking nonsense - really unfamiliar with the framework right now.

PS: you also have a semicolon following .doAfterTerminate(disposeOnceOnly) on a separate line - probably a typo.

[akarnokd]

The Observable can complete outside the method so the classical imperative try-finally doesn't work.

[dimas]

For the completion outside the method you install these doOnTerminate / doAfterTerminate "callbacks". I was only talking on what can happen inside OnSubscribeUsing.call method. You already have something like

    public void call(final Subscriber<? super T> subscriber) {
        try {
            final Resource resource = resourceFactory.call();
            // ... do some stuff ...
            try {
                // ... create Observable via factory
            } catch (Throwable e) {
                // ... release resource and do some sophisticated exception handling ...
                return;
            }
            // ... do some stuff ...
            // ... install doOnTerminate / doAfterTerminate hooks
            try {
                // ... unsafeSubscribe
            } catch (Throwable e) {
                // ... release resource and do some sophisticated exception handling ...
                return;
            }

        } catch (Throwable e) {
            // ... some high level exception handling ...
        }
    }

So my concerns here are:

• you have two try/catch blocks inside that look very similar to me
• still exception can (in theory) happen on a line not covered by those blocks - one of "do some stuff" ones in the example above

So I though maybe these inner try/catch can be avoided completely in favour of one bit try/catch/finally

    public void call(final Subscriber<? super T> subscriber) {
        Resource resource = null;
        try {
            resource = resourceFactory.call();
            // ... do some stuff ...
            // ... do some stuff ...
            // ... install doOnTerminate / doAfterTerminate hooks
            // ... unsafeSubscribe
            subscribed = true;
        } catch (Throwable e) {
            // ... error reporting
        } finally {
            if (!subscribed) {
                // we are returning with exception so subscribe was not successful, dispose resource
            }
        }
    }

it does not have to be finally block given that you catching Throwable - it is safe to release it in "catch".. Just saying...

[akarnokd]

Fixed in #3922. If you think you can clean up the code, PR is welcome.