ReactiveX / rxjs

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

Merge semantics for race conditions between synchronous notifications #422

Closed staltz closed 9 years ago

staltz commented 9 years ago

This is probably a bug.

While making tests for mergeMap I wrote this marble test based on a RxJS 4 test:

  it('should mergeMap many regular interval inners', function () {
   var a =   cold('----a---a---a---(a|)'                    );
   var b =   cold(    '----b---b---(b|)'                    );
   var c =   cold(                '----c---c---c---c---(c|)');
   var d =   cold(                        '----(d|)'        );
   var e1 =   hot('a---b-----------c-------d-------|'       );
   var expected = '----a---(ba)(ba)(ba)c---c---(dc)c---(c|)';

   var observableLookup = { a: a, b: b, c: c, d: d };
   var source = e1.mergeMap(function (value) {
     return observableLookup[value];
   });

   expectObservable(source).toBe(expected);
  });

Notice

var expected = '----a---(ba)(ba)(ba)c---c---(dc)c---(c|)'

This test passes for RxJS 4, but does not pass for RxJS Next, which behaves like this

var expected = '----a---(ba)(ab)(ab)c---c---(cd)c---(c|)'

I quickly took a look and tried to solve this for mergeMap, but the problem seemed deeper, related to subscribeToResult, InnerSubscriber, and potentially from TestScheduler and VirtualTimeScheduler too. Might affect many operators, so was beyond the scope of my mergeMap PR #423.

benlesh commented 9 years ago

I find it puzzling that the order would be (ba)(ba)(ba) I would expect (ab)(ab)(ab). Although I think (ba)(ab)(ab) is even weirder. That (ba) at the beginning is the troubling part.

benlesh commented 9 years ago

Also, with the TestScheduler, there shouldn't be any "race conditions".

staltz commented 9 years ago

Yes. Both existing RxJS 4 and RxJS Next behaviors are puzzling, I'd also expect (ab)(ab)(ab) for both. See here https://github.com/Reactive-Extensions/RxJS/blob/master/tests/observable/selectmany.js#L926-L939

trxcllnt commented 9 years ago

@blesh @staltz I have a hunch this is related to the switch from the default trampoline-style scheduling in RxJS4 to the default recursive-style scheduling in RxJS Next. (ba)(ab)(ab) feels like the pattern I'd expect from such a switch.

It's possible the first (ba) is a bug in how the TestScheduler sorts its scheduled actions. Or alternatively, a and b schedule to dispatch at the same time, but b schedules before a the first time (since subscription to b is synchronous, it gets to schedule its first event ahead of a's second event), but never again. I haven't read the TestScheduler closely enough to say definitively.

benlesh commented 9 years ago

No race conditions to see here, just a stupid developer (me) that thought he should sort actual results. derp derp derp...