Add multicast tests and fix ConnectableObservable. #720
Conversation
|
By the way, @trxcllnt you might want to review this if I'm doing any performance mistake. At least there is one closure introduced there. I focused only on getting the bug fixed, we might want to do a second round to make it performant if there's opportunity for that. |
|
@staltz ... can you rebase this one? |
Add comprehensive marble tests for multicast operator, including retryability, repeatability, refCounting, and multiple observers. Resolves ReactiveX#657.
staltz
force-pushed
the
tests-multicast
branch
from
aad2cfa to
4dcfcc3
Compare
|
I just rebased, but now as you can see, tests fail. And the fix isn't immediate. I'm solving some concurrency issues now in ConnectableObservable. I don't know exactly which change to master happened that affected this. |
| if (++this.refCount === 1) { | ||
| this.connection = connectable.connect(); | ||
| const refCountSubscriber = new RefCountSubscriber(subscriber, this); | ||
| refCountSubscriber.myConnection = this.connection; |
There was a problem hiding this comment.
why is this not added via the constructor above it?
There was a problem hiding this comment.
It can. What's more important is line 101.
|
@Blesh I refactored it. |
|
haha... @staltz, this is perfect, but I have one more request. Can you flatten the fix commits? :) |
|
Yes sir I can. I made them separate in the first place because they hold a lot of story (just check the commit message) on how and why did I make those changes. |
…and refCounting When the ConnectableObservable with refCount always shares the same instance of the underlying subject (such as in publish, publishReplay, publishBehavior), the subscription to the connectable observable should NOT incur additional subscriptions to the underlying cold source. See how tests for publish/publishBehavior/publishReplay were updated to assert that only one subscription to the underlying cold source happens, not multiple, because as soon as the multicasting subject raises an error, this error impedes subsequent subscriptions to the cold source from happening. Fix ConnectableObservable, its connect() method, and the RefCountObservable to support synchronous retry/repeat in the presence of multiple subscribers, and to support retry/repeat in other asynchronous scenarios. Resolves bug ReactiveX#678.
…bsevable Clean up unused code in ConnectableObservable, and optimize some reference lookups.
staltz
force-pushed
the
tests-multicast
branch
from
b4f8426 to
3ac86a3
Compare
|
@Blesh Flat it is. |
|
👍 LGTM ... merging in 3... 2... 1... |
|
That was one hell-ov-a PR. |
Resolves #657 and solves bug #678. Took some time because #678 was truly a nightmare.