Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch() and its shared subscriptions #763

Closed
staltz opened this issue Nov 22, 2015 · 5 comments
Closed

Catch() and its shared subscriptions #763

staltz opened this issue Nov 22, 2015 · 5 comments

Comments

@staltz
Copy link
Member

staltz commented Nov 22, 2015

When writing tests for catch(), I noticed this behavior:

  it('should catch and allow the cold observable to be repeated with the third ' +
  '(caught) argument', function () {
    var e1 =  cold('--a--b--c--------|       ');
    var subs =    ['^                       !',
                   '        ^               !',
                   '                ^       !'];
    var expected = '--a--b----a--b----a--b--#';

    var retries = 0;
    var result = e1
      .map(function (n) {
        if (n === 'c') {
          throw 'bad';
        }
        return n;
      })
      .catch(function (err, caught) {
        if (retries++ === 2) {
          throw 'done';
        }
        return caught;
      });

    expectObservable(result).toBe(expected, undefined, 'done');
    expectSubscriptions(e1.subscriptions).toBe(subs);
  });

Special attention given to this part:

    var e1 =  cold('--a--b--c--------|       ');
    var subs =    ['^                       !',
                   '        ^               !',
                   '                ^       !'];

This happens because CatchSubscriber does

  _error(err) {
    const result = tryCatch(this.selector)(err, this.caught);
    if (result === errorObject) {
      this.destination.error(errorObject.e);
    } else {
      this.add(result.subscribe(this.destination)); // <====
    }
  }

As a consequence, if someone uses source.catch(e => source) as a flavor of retry(), the number of subscriptions will indefinitely grow and stay retained.

What do you think should be done about this behavior?

@kwonoj
Copy link
Member

kwonoj commented Nov 23, 2015

At immediate sight, I'd expect each subscription unsubscribed when next subscription starts, like below.

var subs =    ['^        !',
                '        ^       !',
                '                ^       !'];

staltz added a commit to staltz/RxJSNext that referenced this issue Nov 24, 2015
Fix catch operator to not have anymore a shared underlying Subscription, and instead reset the
subscription for each new observable replacing the caught error. This fixes a potential memory leak
if catch is used as an infinite retry, because subscriptions would be retained since the beginning,
and would increasing each time a catch is performed.

Resolves issue ReactiveX#763.
@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

@staltz ... I believe this was solved with #769, no?

@benlesh benlesh closed this as completed Nov 30, 2015
@kwonoj
Copy link
Member

kwonoj commented Nov 30, 2015

I believe so (though I'm not @staltz :) ) PR still need to be merged though.

staltz added a commit to staltz/RxJSNext that referenced this issue Nov 30, 2015
Fix catch operator to not have anymore a shared underlying Subscription, and instead reset the
subscription for each new observable replacing the caught error. This fixes a potential memory leak
if catch is used as an infinite retry, because subscriptions would be retained since the beginning,
and would increasing each time a catch is performed.

Resolves issue ReactiveX#763.
@benlesh benlesh reopened this Nov 30, 2015
@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

okay... reopening until the merge.

kwonoj pushed a commit that referenced this issue Nov 30, 2015
Fix catch operator to not have anymore a shared underlying Subscription, and instead reset the
subscription for each new observable replacing the caught error. This fixes a potential memory leak
if catch is used as an infinite retry, because subscriptions would be retained since the beginning,
and would increasing each time a catch is performed.

Resolves issue #763.
@kwonoj
Copy link
Member

kwonoj commented Nov 30, 2015

Now really closing issue.

@kwonoj kwonoj closed this as completed Nov 30, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants