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

Observable.concat() does not complete with a buffered scalar observable #1150

Closed
emonkak opened this issue Jan 8, 2016 · 6 comments · Fixed by #1315
Closed

Observable.concat() does not complete with a buffered scalar observable #1150

emonkak opened this issue Jan 8, 2016 · 6 comments · Fixed by #1315
Assignees
Labels
bug Confirmed bug
Milestone

Comments

@emonkak
Copy link

emonkak commented Jan 8, 2016

This Observable does not complete.

const { Observable } = Rx

Observable
  .create(observer => {
    setTimeout(function() {
      observer.next(1)
      observer.complete()
    }, 1000)
    })
  .concat(Observable.of(2))
  .subscribe(
    x => console.log('Next:', x),
    x => console.log('Error:', x),
    () => console.log('Completed')
  )

http://jsfiddle.net/bo6ynqf7/7/

Expected:

Next: 1
Next: 2
Complete

Actual:

Next: 1
Next: 2
@niklasfasching
Copy link

Concat is based on mergeAll-support, which does not subscribe to scalar-observables but instantly nexts their value. However, for notifyComplete to be called, the observable has to be subscribed to by an innerSubscriber.

It can be resolved by simply removing the check & subscribing to non-scalar & scalar observables alike. I.e.

_next(observable: any) {
  if (this.active < this.concurrent) {
    this.active++;
    this.add(subscribeToResult<T, R>(this, observable));
  } else {
    this.buffer.push(observable);
  }
}

But I don't know if that's the best solution...

@kwonoj
Copy link
Member

kwonoj commented Jan 10, 2016

Thinking this might be another side effect of ScalarObservable, related to #1142.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 11, 2016
- remove usage of ScalarObservable for optimization
- implementation of ScalarObservable still remain for possible further usage

closes ReactiveX#1142, ReactiveX#1150, ReactiveX#1140
@trxcllnt
Copy link
Member

@Nupf yes, subscribeToResult already handles ScalarObservable, so any operators that check for _isScalar and aren't using subscribeToResult should be refactored to always call subscribeToResult.

@benlesh benlesh added bug Confirmed bug help wanted Issues we wouldn't mind assistance with. priority: high labels Jan 15, 2016
@benlesh benlesh added this to the 5.0.0-beta.2 milestone Jan 15, 2016
@benlesh
Copy link
Member

benlesh commented Feb 8, 2016

@kwonoj @trxcllnt ... do you know if this is still an issue?

@kwonoj
Copy link
Member

kwonoj commented Feb 8, 2016

Just tried again with TOT and seems it's still occurring.

@benlesh benlesh self-assigned this Feb 8, 2016
@benlesh benlesh added PR: WIP and removed help wanted Issues we wouldn't mind assistance with. labels Feb 8, 2016
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@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
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants