flatMap internal observer unsubscription bug

[TomasStanek]

Hello, I stumbled upon a bug using flatMap in laminar.

Full code:

import com.raquo.laminar.api.L._
import org.scalajs.dom

object Main {

  def main(args: Array[String]): Unit = {
    documentEvents
      .onDomContentLoaded
      .foreach { _ =>
        render(dom.document.body, htmlBody())
      }(unsafeWindowOwner)
  }

  def htmlBody() = {

    val screenWidthVar = Var(dom.document.documentElement.clientWidth)

    dom.window.onresize = _ => {
      val w = dom.document.documentElement.clientWidth
      dom.console.log("resize: " + w)
      screenWidthVar.set(w)
    }

    val screenWidth = screenWidthVar.signal

    val brokenSignal =
      screenWidth
        .map(_ < 1000)
        .flatMap { isSmall =>
          if (isSmall) screenWidth.map("small: " + _)
          else Val("big")
        }

    brokenSignal.foreach(w => dom.console.log("broken: " + w))(unsafeWindowOwner)

    div(child.text <-- brokenSignal)
  }
}

Observed behavior Assuming a screen width larger than 1000, when the page loads, it shows big. (as expected) When the browser is resized down it shows small with the current width and the displayed width is updated. (as expected) When the browser is resized up again, it gets stuck on something like "small: 1023" and then it stops updating. (The expected behavior is to display "big")

Output in console

resize: 884
broken: small: 884
resize: 887
broken: small: 887
resize: 1018
broken: big          <-- this is correct
broken: small: 1018  <-- this isn't
resize: 1141
resize: 1142
resize: 1188

The signal from screenWidth seems to fire/propagate one last time after the signal from Val("big") propagated (or something like that). Obviously, a value small: 1018 with number 1000 or bigger is unexpected as well.

I've observed the same behavior on laminar 0.13.1 and 0.14.2

Is there something I'm doing wrong?

[yurique]

Yeah, this looks incorrect indeed 🤔 , unless I'm missing something, too. ( https://scribble.laminext.dev/u/yurique/cresomiavddhmobizhhjokhvncek ) ( same with windowEvents: https://scribble.laminext.dev/u/yurique/chdkqlnyuiswinceqqsubliiodoh )

[raquo]

Weeeeeird. Good find.

So, there are two main issues here:

1) broken: small: 1018 – this happens because screenWidth.map("small: " + _) receives the event before Airstream manages to kill this subscription. I'm trying to see if there's a way we can avoid this. Currently, when internal observers are removed, they're scheduled for removal not immediately, but at the end of the current transaction, to avoid breaking the iteration over the list of internal observers. If this is the only factor causing this behaviour, I might be able to fix that, but not sure yet.

2) brokenSignal not emitting "small" after Val("big") is emitted – whatever was causing this, I have apparently already fixed it in the next-0.15 branch, so we have at least that going for us. I have reworked a lot of relevant logic in that branch, so it would be hard to figure out what exactly was causing it in <= 0.14.0.

Unfortunately next-0.15 introduces another undesired behaviour – resize: 887 is printed twice. This seems to be related to the same unsubscription delay issue as point 1 above. Looking into that too.

For reference, here's a small, airstream-only reproduction:

    var smallI = -1
    var bigI = -1

    val owner = new TestableOwner

    val intVar = Var(2000)

    val intSignal = intVar.signal

    val brokenSignal =
      intSignal
        .flatMap { num =>
          if (num < 1000) {
            smallI += 1
            intSignal.map("small: " + _).setDisplayName(s"small-$smallI") //.debugLogLifecycle()
          } else {
            bigI += 1
            Val("big").setDisplayName(s"val-$bigI")
          }
        }

    brokenSignal.foreach(v => println(v))(owner)

    def setVar(v: Int): Unit = {
      println(s"\n\nSetting: $v")
      intVar.set(v)
    }

    // --

    setVar(884)
    setVar(887)
    setVar(1018)
    setVar(1141)
    setVar(1142)

[raquo]

I got a PoC working that fixes all issues with this example by making observer unsubscription delay more granular – it only waits until the given observable's iteration of observers is completed, not until the whole transaction is complete. I thought about it for a while, and it seems safe.

This means Airstream will abort the propagation of events faster than before when you unsubscribe from an observable. It's a universal change in semantics, not just for flatMap. In this particular case this change brings desirable results. I hope it doesn't introduce some weird edge cases in other cases, especially in split... Well, all tests pass, so at least there's that.

I'll push this to the next-0.15 branch soon, and the fix will be released in 0.15.0-RC1 in about a month.

---

Meanwhile, @TomasStanek do you need advice on a workaround? In your code you don't actually need a flatMap, so if your full code is similar, you might be able to avoid using it this way. I think the issue is likely because you're using the same signal inside flatMap that you're flatMap-ing over, screenWidth.

Also a few notes on your code snippet:

• You can use Laminar's renderOnDomContentLoaded method instead of subscribing to onDomContentLoaded manually.
• Similar to what Iurii showed, you can use windowEvents.onResize.throttle(20).toWeakSignal.mapTo(dom.document.documentElement.clientWidth) as screenWidth, no need for a Var. Throttling events like resize and scroll is in general a good idea in JS to reduce CPU usage when those events fire, if you don't mind a bit of latency.
• In general you almost never need to use unsafeWindowOwner.

[TomasStanek]

The code snippet is just a full working example demonstrating the behavior. I managed to work around the issue by not using flatMap so it's ok.

Anyway, thank you for your response and tips and I'm looking forward to next-0.15 and 0.15.