Is merge supposed to be symmetric?

[geniusatwrok]

Hello,

Suppose I have two streams like so (using Java notation):

Stream<Foo> sA = ...
Stream<Foo> sB = ...

Now I want to merge them into a Stream<Foo> sC, and if there are simultaneous events I want to use the value from sA:

Stream<Foo> sC = sA.merge(sB, (a, b) -> {System.out.println("simultaneous events occurred"); return a;});

My question is: does the "order" of the merge matter here? E.g. would the above result in the same behavior as:

Stream<Foo> sC = sB.merge(sA, (b, a) -> {...return a;});

Intuitively I thought that the order shouldn't matter. Yet in my program, when I define it using one order, I see the message "simultaneous events" getting printed, but using the other order, I don't! Could I have ran into a bug?

Thanks,

Chris

[the-real-blackh]

Hi Chris,

the order definitely should not matter, so it could be a bug. Could you post the whole falling test case?

Steve

[geniusatwrok]

Sure thing. I wrote this example in the same package as your swidgets examples. When you start it, you will see four buttons. Just keep pressing pass for each player. When it's the End phase and player 2 passes, you should see this in the console:

merge tiebreak. nextPlayer: One newActivePlayer: Two
sChangeActivePlayer: Two
sChangePriority: Two

Now edit the code in the changePriorityStream() function (there's a comment indicating what to swap, should be clear given the context of this discussion). Repeat the above, and instead you'll see:

sChangeActivePlayer: Two
sChangePriority: Two
sChangePriority: One

I.e. the "merge tiebreak" comment does not get printed. It's almost like there's a "glitch": sChangePriority gets a Two and then immediately followed by a One.

Hopefully I'm not wasting your time and this isn't a bug in my code...I'm also not sure how to quote a large chunk of code nicely, so I created a pastebin:

https://pastebin.com/r4WEvWRN

Thanks for looking!

-Chris

[clinuxrulz]

Unbelievable the bug is real. It seems the Set<Node> visited parameter to ensureBiggerThan in "Node.java" that is meant to prevent an infinite loop in the case of a node-cycle is causing the ranks to not be bumped up to the value they are meant to be in some cases.

I.E. change
private boolean ensureBiggerThan(long limit, Set<Nod> visited) {
To:
private boolean ensureBiggerThan(long limit) { (and fix surrounding code to not depend on visited)

Then the bug goes away.

Take the following node graph:

(A)----(B)
 |
 +-----(C)-----(B)

ensureBiggerThan (depth first transversal) gets called then: (numbers are ranks)

(A0)----(B1)
 |
 +------(C1)-----(B1) <-- bug, "visited" set prevents B from having rank 2

[clinuxrulz]

a one liner PR fix is available now for this problem that still prevents infinite loops in the case of node cycles.

fix for http://sodium.nz/t/is-merge-supposed-to-be-symmetric/436

We may want to just throw an exception or print a warning to the console in the case of a node-cycle and just get it out of the way. Just worried it will confuse existing users in older code bases.

Just to be clear. @geniusatwrok 's code is perfectly fine, it was a bug in the Java version of the sodium library.

[the-real-blackh]

Thank you @clinuxrulz!

I have released the Java version of Sodium as 1.2.0. There are a number of bug fixes in it, including this one. It's 1.2.0 because there are additions to the API.

I have released it to the Maven Central repository. At least I think I have. Apparently it take 10 minutes to two hours for it to be synced to the staging site.

[geniusatwrok]

Thanks to you both! @clinuxrulz @the-real-blackh

-Chris