ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.7k stars 3k forks source link

Expand operator behavior #766

Closed kwonoj closed 8 years ago

kwonoj commented 8 years ago

Took example snippet from RxJS 4,

var source = Rx.Observable.return(42)
    .expand(function (x) { return Rx.Observable.return(42 + x); })
    .take(5);

var subscription = source.subscribe(console.log, console.log, ...);

RxJS4

Next: 42
Next: 84
Next: 126
Next: 168
Next: 210
Completed

RxJS Next

RangeError: Maximum call stack size exceeded
    at ExpandSubscriber._next 
benlesh commented 8 years ago

This seems to be more related to take, as when I remove take it works fine.

cc/ @trxcllnt

kwonoj commented 8 years ago

Took a look this bit more, this is behavior of expand. Internally it recursively calling _next() to expand, having 2 problems

 if (result._isScalar) {
          this._next(result.value);
 }

it infinitely continues while there isn't terminating condition.

benlesh commented 8 years ago

for infinite expand with subscription, recursive call causes stack overflow

expand is apparently missing the scheduler argument. We should still default to "no scheduler", which means immediate execution (and potential stack overflow).

kwonoj commented 8 years ago

expand is apparently missing the scheduler argument

: compare to RxJS4, yes it does. I'll try changes for this.

(and potential stack overflow)

: I think this is similar case to https://github.com/ReactiveX/RxJS/issues/651, eventually might need some mechanism to avoid stack overflows.

benlesh commented 8 years ago

eventually might need some mechanism to avoid stack overflows.

That mechanism is allowing users to provide a scheduler.

The problem is that any mechanism (scheduling) to avoid stack overflows also: 1. is less performant, and 2. makes debugging with call stacks gnarly. ... those are two of our top goals.

kwonoj commented 8 years ago

I'm closing this issue since scheduler parameter is being tracked in #841 .