SignalWatcher still execute the effect after element disconnected

[doouding]

Which package(s) are affected?

Other/unknown (please mention in description)

Description

Hi, I'm the member of blocksuite. We recently adopted the preact signal to optimize the data flow. At the same time we found that the lit has provide preact signal integration out of box, so we use this to build our infra. Then we encountered unexpected result.

The SignalWatcher will execute performUpdate even after element disconnected. That cause a bug in which if I delete a block in the editor, the effect will still be executed. It kinda like memory leak but I'm not sure.

Reproduction

You can try it on online blocksuite playground. Steps to reproduction:

1. Click button to switch to whiteboard mode

   

2. Select the blue block and press delete or backspace to delete it

3. Switch back to note mode

4. Type something and hit the enter and repeat

5. You can see the error in the console

   

Workaround

I added isConnected checking in the SignalWatcher.performUpdate before execute the effect. It seems works fine. See https://github.com/doouding/lit/pull/1/files.

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

Failling in lit@3.1.3 and @lit-labs/preact-signals@1.0.2

Browser/OS/Node environment

Browser: Chrome 125.0.6422.141 OS: macOS 13.2.1

[sorvell]

Thanks for pointing this out. There's a tricky issue here which is a bug and can result in a memory leak.

Lit element's update cycle depends only on initial connection to start updating. After that the update cycle remains active even if the element is disconnected. This means, for example, if an update is pending when an element is disconnected, it will complete after the element is disconnected.

In general, this is fine and by design, but in SignalWatcher, the disconnection is used to remove any signal subscriptions, effectively to turn off signals from updating the element. However, as previously noted, if an update is pending while the element is disconnected, it will complete. And in performing an update SignalWatcher subscribes to any signals that it accesses. Therefore, when an update is pending during disconnection, the result is that the disconnected element retains subscriptions to any signals it accesses during its disconnected update.

This is almost certainly unexpected and likely to result in a memory leak.

The simplest fix is to have SignalWatcher complete the update cycle synchronously (via a call to performUpdate) in disconnectedCallback before disposing of signal subscriptions.

Here's a playground that sets up this scenario and implements the above fix.

[justinfagnani]

I'm not sure if we should do a sync update on disconnected. I think ideally we would match the behavior of plain ReactiveElements and updates during disconnected.

You should be able to get the same scenario without SignalWatcher by setting a reactive property, then disconnecting the element. There will be a pending update, which will be applied while disconnected:

el.foo = 'abc';
el.remove();

So while flushing pending updates on disconnect in SignalWatcher might workaround this specific issue, it doesn't solve the problem in general. The component that's erroring here could have an update enqueued some other way while disconnected and throw the same exception.

Pedantically, I think the for correctness that elements that need to rely on connected state for their render should guard the computation on this.isConnected. I know that may be cumbersome to do everywhere though, so where this is an issue, I'd rather find a fix that works regardless of the source of the update. Maybe we need a change in ReactiveElement?

[sorvell]

I'm not sure if we should do a sync update on disconnected.

Just for clarity, the proposed fix only performs a sync update if one is pending during disconnection.

You should be able to get the same scenario without SignalWatcher

The issue is the memory "leak," and while you can have that without SIgnalWatcher, retaining memory as part of the update is pretty specific to it.

Maybe we need a change in ReactiveElement?

I'm open to that, but would this be non-breaking?

disconnectedCallback() {
  if (!this.isConnected) {
    this.performUpdate();
  }
}

[sorvell]

After some discussion, we're inclined not to change this behavior.

While disconnection is used as a signal to unsubscribe from signal based updates, an element can request an update while disconnected, and if it does so, it will update and subscribe to any signals it accesses.

In general, code that assumes a specific connection state should validate that state by checking this.isConnected. Ideally this is done explicitly where needed, but it could also be via shouldUpdate() { return this.isConnected; }.

[justinfagnani]

I think the big thing to take away here is that this problem can occur without signals at all. If you trigger an update and disconnect the element before the update runs, then code that assume there's a parent will fail.

Like @sorvell said, you can check this.isConnected, but you can also just check that the objects you use are available. Like this.parentElement?.offsetHeight, etc. TypeScript should help there.

Another thing you might consider, though it could get a bit complicated, is deferring your disconnected clean up code a microtask, if you happen to be doing your own cleanup. This can be good to avoid unnecessary costs if elements are disconnected then immediately reconnected, as happens when things are moved around the DOM. But it also will mean that cleanup will run after a pending update that was enqueued while connected. Whether that's good or not depends on the nature of the resources you're cleaning up.

One way to defer is to use queueMicrotask():

disconnectedCallback() {
  queueMicrotask(() => {
    if (!this.isConnected) {
      // only clean up if the element wasn't immediately reconnected.
      this.#cleanup();
    }
  });
}