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

retry: fixes and tests #547

Closed
wants to merge 2 commits into from

Conversation

staltz
Copy link
Member

@staltz staltz commented Oct 16, 2015

Resolves (half of) #546.

Andre Medeiros added 2 commits October 16, 2015 19:04
Fix retry operator to unsubscribe from the source whenever the
source emits an error and a new retry will be attempted. Also fix the
operator to unsubscribe the internal retried subscription to the source
whenever the result Observable is unsubscribed.

Resolves issue ReactiveX#546.
@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2015

What happens if resubscription occurs sufficiently large, i.e) invoked with retry() without param to allow indefinite retry and source fails more than several thousands (>10000 maybe) ?

@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2015

Though I think that's sort of extreme cases usually do not expect to happen in real.

@staltz
Copy link
Member Author

staltz commented Oct 16, 2015

What happens if resubscription occurs sufficiently large, i.e) invoked with retry() without param to allow indefinite retry and source fails more than several thousands (>10000 maybe) ?

In marble tests there is a time limit, maxFrames = 750. However, if the source is cold('#'), then the retries will happen synchronously and the time frame will not progress at all in the TestScheduler, so it's possible to put the test in an infinite loop until the test runner breaks.

@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2015

Actually question was about not creating test cases, trying with actual code such as Observable.throw().retry(LAAARGE) (pseudo, of course) not run by testscheduler. Just curiosity though.

@staltz
Copy link
Member Author

staltz commented Oct 16, 2015

With RxJS 4, Observable.throw().retry() (indefinite) throws the error:

"Maximum call stack size exceeded"

I'd expect RxJS Next to do the same.

@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2015

Interesting, thanks for confirmation.

@staltz
Copy link
Member Author

staltz commented Oct 16, 2015

Actually, sorry, that was RxJS 2. With RxJS 4, the page freezes. Probably doing an infinite loop.

@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2015

I just did quick try with RxJS4 + node myself and behavior was same, it seems does infinite retrying. I can give a try with this changes once it's merged. Again, thanks! :)

@benlesh
Copy link
Member

benlesh commented Oct 16, 2015

@staltz ... this is precisely the solution I was thinking of when I first found this issue the other day. Nice work.

@benlesh
Copy link
Member

benlesh commented Oct 16, 2015

Merged with 09b475a..

Also, I had to pin eslint to the original version we had installed, I had wiped out my node_modules directory and npm installed, and it installed eslint 1.7.0, which was giving false positives on our spec files. I just pinned it to a version I knew would work with our build.

@staltz
Copy link
Member Author

staltz commented Oct 16, 2015

Good!

@staltz staltz deleted the fix-retry-unsubscription branch October 16, 2015 20:58
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants