Skip to content

Commit

Permalink
Fix: Bug in JsPriorityQueue implementation that produced glitches in …
Browse files Browse the repository at this point in the history
…complex cases.

Specifically, when two or more observables were pending in the same transaction at the same time, if one of them actually synchronously depended on the other, it could fire ahead of the observable that it depended on, causing a glitch. Whether this actually happened also depended on the order of (internal) observers in the observable graph, you could avoid this bug just by luck. This bug could affect nested combineWith or delaySync observables, resulting in extraneous events (glitches) in case of combineWith, or observables firing in the wrong order (in case of delaySync).

See https://gitter.im/Laminar_/Lobby?at=6007655b36db01248a8bf5a9 for the original bug report.
  • Loading branch information
raquo committed Jan 21, 2021
1 parent b4c4eae commit a5d36a8
Show file tree
Hide file tree
Showing 3 changed files with 389 additions and 6 deletions.
13 changes: 7 additions & 6 deletions src/main/scala/com/raquo/airstream/util/JsPriorityQueue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,22 @@ package com.raquo.airstream.util

import scala.scalajs.js

// @TODO[Test] add tests for this class

class JsPriorityQueue[A](getRank: A => Int) {

private[this] val queue: js.Array[A] = js.Array()

def enqueue(item: A): Unit = {
val itemRank = getRank(item)
var insertAtIndex = 0
var foundHigherRank = false
while (
insertAtIndex < queue.length &&
!foundHigherRank
insertAtIndex < queue.length && !foundHigherRank
) {
if (getRank(queue(insertAtIndex)) <= getRank(item)) {
if (getRank(queue(insertAtIndex)) > itemRank) {
foundHigherRank = true
} else {
insertAtIndex += 1
}
insertAtIndex += 1
}
queue.splice(index = insertAtIndex, deleteCount = 0, item) // insert at index
}
Expand All @@ -40,4 +39,6 @@ class JsPriorityQueue[A](getRank: A => Int) {
@inline def isEmpty: Boolean = size == 0

@inline def nonEmpty: Boolean = !isEmpty

def debugQueue: List[A] = queue.toList
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,100 @@ class SyncDelayEventStreamSpec extends UnitSpec {
calculations.clear()
effects.clear()
}

it("multi-level, nested, dependent sync observables") {

// See https://gitter.im/Laminar_/Lobby?at=6007655b36db01248a8bf5a9 and below

implicit val testOwner: TestableOwner = new TestableOwner

val v = Var(0)

val calculations = mutable.Buffer[Calculation[Int]]()

val signal1 = v.signal
.map(Calculation.log("signal1", calculations))

val signal2 = signal1
.map(ev => ev)
.map(Calculation.log("signal2", calculations))

val signal3 = signal2
.composeChanges { signal2Changes =>
signal2Changes
.delaySync(signal1.changes)
.map(Calculation.log("signal3-changes-source", calculations))
}
.map(Calculation.log("signal3", calculations))

val signal4 = signal1
.composeChanges { signal1Changes =>
signal1Changes
.delaySync(signal3.changes)
.map(Calculation.log("signal4-changes-source", calculations))
}
.map(Calculation.log("signal4", calculations))

val signal5 = signal2
.composeChanges { signal2Changes =>
signal2Changes
.delaySync(signal3.changes)
.map(Calculation.log("signal5-changes-source", calculations))
}
.map(Calculation.log("signal5", calculations))

// --

// Order is important
signal5.addObserver(Observer.empty)
signal4.addObserver(Observer.empty)
signal3.addObserver(Observer.empty)

// signal4's and signal5's initial value does not depend on signal3's initial value,
// only on its changes, so it's ok that signal3's initial value is evaluated later.
assert(calculations.toList == List(
Calculation("signal1", 0),
Calculation("signal2", 0),
Calculation("signal5", 0),
Calculation("signal4", 0),
Calculation("signal3", 0)
))

calculations.clear()

// --

v.set(1)

assert(calculations.toList == List(
Calculation("signal1", 1),
Calculation("signal2", 1),
Calculation("signal3-changes-source", 1),
Calculation("signal3", 1),
Calculation("signal5-changes-source", 1),
Calculation("signal5", 1),
Calculation("signal4-changes-source", 1),
Calculation("signal4", 1)
))

calculations.clear()

// --

v.set(2)

assert(calculations.toList == List(
Calculation("signal1", 2),
Calculation("signal2", 2),
Calculation("signal3-changes-source", 2),
Calculation("signal3", 2),
Calculation("signal5-changes-source", 2),
Calculation("signal5", 2),
Calculation("signal4-changes-source", 2),
Calculation("signal4", 2)
))

calculations.clear()

}
}
Loading

0 comments on commit a5d36a8

Please sign in to comment.