Repeat operator cannot repeat indefinitely

[kwonoj]

Code snippet
Rx.Observable.of(42).repeat(-1).subscribe(console.log);

eventually will stop unexpectedly based on runtime due to maximum call stack exceeded. This is happening due to current resubscription occurs recursively. In previous implementation I used scheduler to disconnect recursive chain at the moment lacks of better idea.

I'm assuming retry will behaves same since it's using same mechanics.

[kwonoj]

As discussed in #547, I think this is bit extreme cases though.

[benlesh]

this is true for retry and repeat... when you're applying those to completely synchronous observables, you're going to get that because the default scheduler is recursive. The only other thing we could do would be to force the resubscription to happen on the trampoline scheduler in these operators. I think I discussed this with @trxcllnt and he convinced me we didn't want to do that.

Perhaps he can remind me what the reasoning is?

[benlesh]

As of right now, I'm inclined to say this isn't an issue. If someone really wants to repeat a completely synchronous observable indefinitely, they should really use a subscribeOn directly before the repeat call.

What do you think @kwonoj? Is that answer satisfactory?

[kwonoj]

I agree. As already commented, I believe this is extreme cases that usually would not attempted to be used. Issue was created to confirm behavior to check difference between RxJS4.

[kwonoj]

Maybe closing issue for now?

[mattpodwysocki]

@kwonoj @Blesh no, it's a bug, keep it open. It's not an extreme case at all, there's a bug with their scheduler as RxJS v4 does not have this issue

[benlesh]

@mattpodwysocki okay.

[kwonoj]

Do we consider this issue to be handled as Scheduler side behavior? (maybe changing subj.?)

[benlesh]

It's a result of default recursive scheduling on synchronous observable types. Where RxJS 4 used trampoline scheduling by default, we're using recursive scheduling for performance reasons. The side effect to this is that if you retry or repeat more than 255 times, you blow the stack.

Perhaps we could implement some sort of progressive scheduling in these scenarios. But I'm just spitballing... I'd like @trxcllnt's opinion on the matter.

[trxcllnt]

There is not a "bug with the scheduler," there is no scheduler (just like in the rest of the library).

@Blesh is right, if you want to indefinitely repeat a synchronous Observable, you should use subscribeOn. Perhaps repeat or retry can detect "infinite" repeats, and use subscribeOn internally.

The real bug would be to shift a sequence onto a Scheduler the developer didn't choose just because an edge-case exists. Don't stop people from doing the wrong thing; make it easy for them to do the right thing, etc.

[roganov]

Not exactly related to this issue, but would there be a way to make a cold observable 'lazy'?
For example, in RxJS4 the following code would freeze the browser:

someObservable.zip(Observable.range(1, Number.MAX_SAFE_INTEGER)).subscribe()

I can switch to default scheduler, but then it would take enormous CPU resources.

someObservable.zip(Observable.range(1, Number.MAX_SAFE_INTEGER, Rx.Scheduler.default)).subscribe()

I'm sorry in case it's a wrong place to ask.

[trxcllnt]

@roganov cold Observables are lazy, meaning they don't do anything until you call subscribe. Your example is equivalent to a for-loop with max safe int as the upper-bounds. If you're looking for an infinite pull-stream, you can find them in Interactive-Extensions.

[mattpodwysocki]

@trxcllnt from my experience there should be no stack overflows with any sequence (of course non-shared) which would indicate a bug, hence my comment. That's why we trampoline to avoid such problems in RxJS v4

[trxcllnt]

@mattpodwysocki yes, except we've built in the "recursive" scheduling strategy (RxJS 4's Immediate Scheduler) by defaulting to imperative loops in all Observable factories. Thus, if someone doesn't dispatch on a scheduler in their source Observable, they're implicitly buying in to the recursive strategy. This means the behavior of repeat will, by default, mimic RxJS v4 if you use the recursive scheduler, with the following differences (assuming max 1000 stack-depth):

// fails after the 999th *repeat* in RxJS v5:
Observable.range(0, 10000).repeat(1000).subscribe();

// fails after the 999th *onNext* (before any repeating can occur) in RxJS v4:
Observable.range(0, 10000, Rx.Scheduler.immediate).repeat(1000).subscribe();

[trxcllnt]

@mattpodwysocki essentially, a large part of the performance improvement is based in the decision to fall-back to loops instead of trampolining, and we didn't want to force a different scheduling strategy than the user requested.

[benlesh]

Thus, if someone doesn't dispatch on a scheduler in their source Observable

This AND, if someone writes an Observable that isn't doing anything async... Which is sort of a "why are you doing this?" scenario. I think it would be a really, really strange choice to use Observable for an entirely synchronous process that could error that you want to retry. And again, an entirely synchronous process that repeats forever? Stack overflow or not, you're going to lock the thread.

[benlesh]

we didn't want to force a different scheduling strategy than the user requested.

@trxcllnt ... do you think in the case that a user says repeat more than say, 100 times, we can assume they might want to schedule resubscription and handle that for them?

Alternatively, perhaps we could add a second parameter for people to specify a resubscription scheduler. (we could do the same on catch, retry and retryWhen, too)

Again, I really think this is only going to affect people that are attempting to heavy-handed retries on a completely synchronous observable, which is an obvious gaff because it will lock the thread.

[benlesh]

closing for inactivity.