[Bug] Regression in 5.6.0 concerning auto-tracking

[boris-petrov]

🐞 Describe the Bug

A couple of my tests started blowing up with the update-after-read assertion after upgrading from 5.5.0 to 5.6.0. Still happens in 5.7.0.

🔬 Minimal Reproduction

Repo

Clone, pnpm install, ember serve, open the page, look at it blow up (in the console) one second later. Downgrading to 5.5.0 fixes the problem. Also, funnily, adding {{#if this.results}}{{/if}} (or anything relating this.results) in parent.hbs also makes it stop blowing up.

😕 Actual Behavior

Autotracking assertion.

🤔 Expected Behavior

No assertion.

🌍 Environment

• Ember: 5.6.0+
• Node.js/npm: v21.7.0
• OS: Linux
• Browser: Chromium

cc @NullVoxPopuli

P.S. Note this seems to happen because of the style modifier. If it is removed, the code works fine even though it's "touching" the same things. Not sure why...

[NullVoxPopuli]

note that the repro is called ember-resources-bug but ember-resources isn't used here :sweat_smile:

it's using ember-could-get-used-to-this, which uses a side-effecting callbacks on a class as part of the Helper lifecycle.

@boris-petrov and I talked about this on Discord a good bit, and I think that the lack of error was actually a bug, but what's goofy with this repro is that "the work-around" needs an explanation -- which I haven't looked in to why ends up working out that way -- but seems suspicious.

This is the resource in the repro:

import { tracked } from '@glimmer/tracking';
import { Resource } from 'ember-could-get-used-to-this';

export default class LiveSearchResource extends Resource {
  @tracked results;

  get value() {
    return this.results;
  }

  setup() {
    this.update();
  }

  update() {
    this.results = [this.args.named.counter];
  }
}

which, I would expect that setting tracked data synchronously during create/update would cause a back-tracking assertion, as create and update in HelperManager (capabilities 3.23+) have auto-tracked create/update calls.

The Todos:

• [ ] find an explanation for why {{#if this.results}}{{/if}} gets around the infinite re-render assertion
• [ ] try to find which of the VM changes from 5.5 -> 5.6 caused the change in behavior

[boris-petrov]

Ah, sorry for the wrong name! Yes, the "resources" in the name is the generic Ember concept, not the specific implementation of @NullVoxPopuli. :)

We did discuss at length, yes. However, I'm still not convinced that @NullVoxPopuli is right in that particular case. As I mentioned in the conversation with him:

I think the resource is created, setup is called and only then get value is called. Same for update - it is called first and then get value.

Which is a read-after-write which should not trigger the assertion.

Also, there is the fact that removing the style modifier call - the code works fine. Even though the same things happen - the same values get "touched" at the same times. Just no modifier call.

Thirdly, there is this strangeness with {{#if this.results}}{{/if}}.

I might be wrong about all this, of course. You're the Ember gurus. :)

[NullVoxPopuli]

do you have a more minimal reproduction that doesn't use ember-could-get-used-to-this?

[boris-petrov]

Sorry, no, I would have given if it was so easy. You can 1) try with ember-resources instead of ember-could-get…; 2) copying the code of ember-could-get… locally in the reproduction and that would help with debugging I guess as you can modify it.

Message ID: @.***>

[ef4]

Following up from a question on https://github.com/glimmerjs/glimmer-vm/issues/1762#issuecomment-3284261182:

This is the same class of problem. In the reproduction, the LiveSearchResource is updating @tracked results in response to other tracked state (this.args.named.counter) changing. Which is generally unsafe.

Attempting to reason about what order reads will happen during rendering is not the right way to think about this kind of problem. I think the actual text of the assertion is misleading people, in that it talks about "read after write", when really I would prefer it to say "write during render". Any write during render is probably unsafe, because it requires you to make assumptions about what order glimmer is going to update things. And the order is not guaranteed to never change.

As written, the fix would be to avoid creating a new @tracked root of state, since there's not actually any state in the resource (it's all derived from tracked state elsewhere):

- @tracked results;
+ get results() {
+  return [this.args.named.counter];
+ }
// ...
- update() {
-  this.results = [this.args.named.counter];
- }

But I realize the reproduction might be intentionally simplified. I'm guessing the real reason you have a resource here is that it does asynchronous work. In which case there's not really a bug as long as you always do the async work before updating the tracked results. I made this edit to simulate async work and there's no assertion:

diff --git a/app/helpers/test-resource.js b/app/helpers/test-resource.js
index 4c121aa..d698ce1 100644
--- a/app/helpers/test-resource.js
+++ b/app/helpers/test-resource.js
@@ -2,7 +2,7 @@ import { tracked } from '@glimmer/tracking';
 import { Resource } from 'ember-could-get-used-to-this';
 
 export default class LiveSearchResource extends Resource {
-  @tracked results;
+  @tracked results = [-1];
 
   get value() {
     return this.results;
@@ -12,7 +12,10 @@ export default class LiveSearchResource extends Resource {
     this.update();
   }
 
-  update() {
+  async update() {
+    // simulating async work
+    await Promise.resolve();
+
     this.results = [this.args.named.counter];
   }

In this case, something must be chosen as the initial value of the resource before the asynchronous work has finished. I just picked [-1] because it makes the existing reproduction run. But a more complete resource would probably expose an isLoading property that consumers would need to check before assuming the data is present.

A small subtlety here is that initialization of a tracked field is not the same as updating it. The assignment in @tracked results = [-1] is safe even though it technically happens during render because by definition, nothing can ever read the value of the field before initialization.

[NullVoxPopuli]

as for async stuff, we also kind of have some simpler abstractions now (resources are fairly low-level (something I'm hoping to touch on at emberfest!)), such as

• https://reactive.nullvoxpopuli.com/functions/get-promise-state.getPromiseState.html
• https://warp-drive.io/api/@warp-drive/core/reactive/functions/getPromiseState

This encapsulation protects you from encountering the "write during render" assertion (I agree with the phrasing change! let's do it)

[boris-petrov]

@ef4 thank you very much for the detailed answer! I believe it makes sense to me and I've fixed the problem in my case. But as @simonihmig mentioned in the other issue, it would be nice to have a way to trigger the assertion in all broken cases. My tests hit this code tens of thousands of times each run and only a couple of them hit this problem. And, as was mentioned, it was definitely not obvious what the trouble was.

Thanks again for the time! I'll close this issue.

[NullVoxPopuli]

implementation-wise, it may be tricky to do, because lazy-initialization during render is something we (neededly) allow. 🤞 something better is achievable tho -- we def need better error-driven guidance