Killed binding fires again

[Starzu]

Tested on 0.8.0-RC2.

val p = Property(1)
val s = SeqProperty(1,2,3)

div(
  produceWithNested(p) { case (v, nested) =>
    div(
      nested(repeat(s) { v2 =>
        println((v, v2.get))
        div().render
      })
    ).render
  }
).render

CallbackSequencer().sequence {
  p.set(2)
  s.set(Seq(4,5,6))
}

The code above prints:

(1,1) 
(1,2)
(1,3)
(2,4)
(2,5)
(2,6)
(1,4)
(1,5)
(1,6)

The last 3 lines are unexpected. It seems CallbackSequencer keeps the listener from SeqPropertyModifierUtils:108 and the handlePatch method does not check if the binding was already killed.

[ddworak]

I've analysed this issue thoroughly and I don't think it's as simple as it seems. CallbackSequencer only works at the level of single properties to avoid duplicate listener fires. The behaviour you've noticed is not limited to SeqProperty or nested repeats, e.g.:

      val p = Property(1)
      val s = Property(2)

      div(
        produceWithNested(p) { (v, nested) =>
          div(
            nested(produce(s) { v2 =>
              println(v -> v2)
              div().render
            }
            )).render
        }
      ).render

      CallbackSequencer().sequence {
        s.set(4)
        p.set(3)
      }

prints:

(1,2)
(1,4)
(3,4)

In this case, both listeners have to trigger at least once and the sequencer handles duplication case correctly (adding another .set to these properties does not increase the number of fires).

Please notice:

      val fired = ListBuffer.empty[String]
      val p = Property(1)
      val s = SeqProperty(1,2,3)

      val combined = s.combine(p)((v1, v2) => v1 -> v2)

      combined.listen(s => fired +=  s.toList.toString())

      CallbackSequencer().sequence {
        p.set(2)
        s.set(Seq(4,5,6))
      }

      println("results: " + fired.mkString("\n"))

only fires once with correct value.

The example you've provided doesn't actually trigger "old" listeners, it fires the newly created ones. It seems that we can avoid this by skipping listeners created after queuing altogether. We cannot just copy the list earlier since we need to allow cancelling during callback processing. Please check proposed solution in #283

[Starzu]

@ddworak I've played a little with this fix and I've got two snippets for you.

The first one:

val p = Property(2)
val s = Property(3)

div(
  produceWithNested(p.transform(_ + 1)) { case (v, nested) =>
    div(
      nested(produce(s) { v2 =>
        println((v, v2))
        div().render
      })
    ).render
  }
).render

CallbackSequencer().sequence {
  p.set(20)
  s.set(30)
}

CallbackSequencer().sequence {
  s.set(300)
  p.set(200)
}

It prints:

(3,3)
(3,30) // ???
(21,30)
(21,300) // ???
(201,300)

The second seems to be a bug introduced in #283. You cannot remove these copyArray calls, because cancel modifies the listeners collection while you are iterating over it.

val t = Property(10)
val regs = ArrayBuffer.empty[Registration]
regs += t.listen(_ => { println("1"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("2"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("3"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("4"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("5"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("6"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("7"); regs.foreach(_.cancel()) })
regs += t.listen(_ => { println("8"); regs.foreach(_.cancel()) })
t.touch()

It throws an exception: "An undefined behavior was detected: undefined is not an instance of scala.Function1".

[ddworak]

The second is obviously a bug which our test suite didn't cover and I didn't check this case in JS (it works on the JVM due to mutable.ArrayBuffer guarantees). Using it on the frontend in these cases adds a small, constant overhead (~30 nanos), which we would still incur by adding another copy of the array. Fix: #285

The first one is a non-issue and I'll have to consider it more thorougly. In the example, the (21,300) case is not avoidable, but the (3, 30) should be (as we've seen in previous changes). The callbacks are sequenced correctly there according to our current logic - the listeners are on a derived/forwarded property, therefore its callbacks are queued and executed at the end.
We can consider pinning callbacks for derived properties to origin, but I'm not sure it's possible in all cases yet.

[ddworak]

Closed in favour of #290