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

Merge semantics for race conditions between synchronous notifications #422

Closed
staltz opened this issue Sep 30, 2015 · 5 comments · Fixed by #473
Closed

Merge semantics for race conditions between synchronous notifications #422

staltz opened this issue Sep 30, 2015 · 5 comments · Fixed by #473

Comments

@staltz
Copy link
Member

staltz commented Sep 30, 2015

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
Copy link
Member

benlesh commented Sep 30, 2015

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
Copy link
Member

benlesh commented Sep 30, 2015

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

@staltz
Copy link
Member Author

staltz commented Sep 30, 2015

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
Copy link
Member

trxcllnt commented Oct 6, 2015

@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
Copy link
Member

benlesh commented Oct 7, 2015

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

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

Successfully merging a pull request may close this issue.

3 participants