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

fix(expand): terminate recursive call when destination completes #793

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Nov 25, 2015

relates to #766

Partially resolves expand operator behavior by adding recursive call terminating condition. Still inifnite expand causes stack overflow though.

Feeling this might not be best way to solve issue, any suggestions would be gladly welcome.

@staltz
Copy link
Member

staltz commented Nov 26, 2015

@kwonoj this PR might make sense to merge together with #788. With both PRs, the infinite expand of ScalarObservables causes stack overflow eventually, but at a very large number, 579096.

If I add a small delay (hence converting the synchronous ScalarObservable to a normal async Observable), like .expand(function (x) { return Rx.Observable.of(42 + x).delay(1); }), then the infinite expand can easily pass larger numbers like 1000000 and keep on going indefinitely.

So, I don't think very long living synchronous expands is something we need to support. This PR and PR #788 should be enough.

@kwonoj
Copy link
Member Author

kwonoj commented Nov 26, 2015

I agree this PR need to be merged with #788 to correct its behaviors.

Supporting infinite expand is somewhat debatable, I'm thinking this is similar cases to #651 . In general, with current design operators have weakness to repeat its behavior with sufficiently large number (depends on system, runtime). It would be better to have mechanism instead of recursive calling.

@staltz
Copy link
Member

staltz commented Nov 26, 2015

Mmm, yes, agreed.

@kwonoj kwonoj merged commit 3b8cf94 into ReactiveX:master Nov 30, 2015
@kwonoj
Copy link
Member Author

kwonoj commented Nov 30, 2015

Merged with 3b8cf94.

@kwonoj kwonoj deleted the fix-expand branch November 30, 2015 20:37
@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.

2 participants