Potential resource leak in unsubscribe loop

[briancavalier]

Subscription builds up an array of subscriptions in add(), which is later torn down in a loop when unsubscribe() is called. It seems that if a particular subscription.unsubscribe call throws, the subscriptions later in the array will never be unsubscribed. If those subscriptions represent resources, perhaps from Observable.create calls as mentioned in Guidelines bullet 4, it appears that the resources would be leaked.

Is there a potential leak there?

[trxcllnt]

@briancavalier yes there is. unsubscribe should be called safely.

[benlesh]

Thanks for pointing that out @briancavalier! Nice of the author of Most.js to lend a hand. 💖

[trxcllnt]

@Blesh @briancavalier upon further review, this may not be so clear cut, and I have a feeling this is related to #1186.

When an error is thrown from the private _unsubscribe method, what should we do with it? We could:

1. let it interrupt execution, potentially leaking memory.
2. catch it, continue disposing, and re-throw the error at the end

If we take the second approach, we'll also have to wrap the unsubscribe calls on our child subscriptions, since their unsubscribe methods may also throw. Subscription's unsubscribe method would look something like this:

  unsubscribe(): void {

    if (this.isUnsubscribed) {
      return;
    }

    this.isUnsubscribed = true;

    let errors: Array<any>;
    let errorHappened = false;

    const { _unsubscribe, _subscriptions } = (<any> this);

    (<any> this)._subscriptions = null;

    if (isFunction(_unsubscribe)) {
      try {
        _unsubscribe.call(this);
      } catch (e) {
        errorHappened = true;
        (errors || (errors = [])).push(e)
      }
    }

    if (isArray(_subscriptions)) {

      let index = -1;
      const len = _subscriptions.length;

      while (++index < len) {
        const subscription = _subscriptions[index];
        if (isObject(subscription)) {
          try {
            subscription.unsubscribe();
          } catch (e) {
            errorHappened = true;
            (errors || (errors = [])).push(e)
          }
        }
      }
    }

    if (errorHappened) {
      if (errors.length > 1) {
        throw errors;
      } else {
        throw errors[0];
      }
    }
  }

As you can see, it's a little funny. What do we do if we encounter many errors disposing our child subscriptions? re-throw them individually? as a group?

I know this is goes against my opinion in #1186, but perhaps the best thing to do is nothing, and provide guidance that users shouldn't throw errors from their unsubscribe methods.

[benlesh]

@trxcllnt throw them as a group, I'd say. I have a PR ready to do that covers at least some of the leak. I need to add more tests to be sure, though.

[briancavalier]

No problem. Thanks for looking into it, @trxcllnt and @Blesh.

[trxcllnt]

@Blesh fyi: https://github.com/Reactive-Extensions/RxJS/tree/master/doc/designguidelines#515-unsubscription-should-not-throw

[benlesh]

Because of this, exceptions in unsubscriptions should be avoided.

I don't think it would be responsible for us to just say "don't throw in your unsubscribes" and not handle the unsubscriptions as we have in this PR.

[mattpodwysocki]

@Blesh I think it's perfectly responsible as we laid it out in our design document which we thought over for months. cc @headinthebox

[benlesh]

Seems like a possible source for memory leaks that we just fixed at no cost to performance or ergonomics. I'd hate to remove the fix without a good reason.

[mattpodwysocki]

It's the same rule as in any language, you don't throw in finalizers, simple as that

[benlesh]

It's the same rule as in any language, you don't throw in finalizers, simple as that

I'm unfamiliar with this rule or how it applies to this method call.

Perhaps I'm just confused: Are you asking us to remove the memory leak protection code?

Because currently, it will:

1. prevent memory leaks by completing unsubscriptions
2. then throw synchronously

and if we remove it:

1. Allow memory leaks
2. then throw synchronously

... It seems pretty clear we should keep the code in there. We're protecting ourselves from user-land errors, since the unsubscribe function could be provided by users of the type.

[benlesh]

FWIW: If we do decide to remove this protection code, then we should also remove the code that is trying to unsubscribe after errors in user-provided nextHandler. Because @jhusain's argument (to me personally) against that functionality was basically that users should be handling their own errors in their code... which is sounds is like what you're saying here.

Because this is very, very similar to what we're doing and talking about in #1186... and there's still some hard work to go there (Involving errors thrown in the same call stack in a next handler during subscription phase and before the unsubscribe logic is even returned).

[mattpodwysocki]

@Blesh I'm with @jhusain on this, it is up to the user not to do the wrong thing here as no finalizer should ever throw. It should be a fatal event anyhow for anything that causes errors in finalization, period, so it's not a leak more than it is a crash.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.