Skip to content

fix: exclude derived writes from effect abort and rescheduling #16477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

paoloricciuti
Copy link
Member

Closes #16423

One possible strategy to fix this issue (maybe not the best ?). We keep track of how many derived_writes we have before updating the effect and we subtract that number from the write_version to have check if something else increased it (namely setting a derived or a source).

One thing that I would've loved to confirm but failed to do so is check that the test in #16280, if rewritten with deriveds, still pass. But that's very convoluted and I was not able to find a way to rewrite it so that the derived was updated in the effect.

Let's discuss if this is a good strategy.

@dummdidumm also had the idea to use even and off numbers for write_version for deriveds and sources but that sounds a nightmare to debug.

Another idea I had was literally just a flag on the effect when internal_set is called.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Jul 22, 2025

🦋 Changeset detected

Latest commit: ab32c96

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@16477

@Rich-Harris
Copy link
Member

Something I've been working on separately, that would fix this and #16413, is to clear out the current_batch after we've traversed the effect tree and before flushing queued effects, such that any changes inside those queued effects result in a new batch being created.

That would allow us to do this instead of tracking the number of derived writes:

/**
 * @param {Array<Effect>} effects
 * @returns {void}
 */
function flush_queued_effects(effects) {
  var length = effects.length;
  if (length === 0) return;

  var i = 0;

  while (i < length) {
    var effect = effects[i++];

    if ((effect.f & (DESTROYED | INERT)) === 0 && is_dirty(effect)) {
-     var wv = write_version;

+     // if state is written in a user effect, abort and re-schedule, lest we run
+     // effects that should be removed as a result of the state change
+     if (current_batch !== null) {
+       break;
+     }

      update_effect(effect);

      // Effects with no dependencies or teardown do not get added to the effect tree.
      // Deferred effects (e.g. `$effect(...)`) _are_ added to the tree because we
      // don't know if we need to keep them until they are executed. Doing the check
      // here (rather than in `update_effect`) allows us to skip the work for
      // immediate effects.
      if (effect.deps === null && effect.first === null && effect.nodes_start === null) {
        // if there's no teardown or abort controller we completely unlink
        // the effect from the graph
        if (effect.teardown === null && effect.ac === null) {
          // remove this effect from the graph
          unlink_effect(effect);
        } else {
          // keep the effect in the graph, but free up some memory
          effect.fn = null;
        }
      }

-     // if state is written in a user effect, abort and re-schedule, lest we run
-     // effects that should be removed as a result of the state change
-     if (write_version > wv && (effect.f & USER_EFFECT) !== 0) {
-       break;
-     }
    }
  }

  while (i < length) {
    schedule_effect(effects[i++]);
  }
}

I think this is probably a more 'correct' fix (insofar as current_batch !== null translates to 'strap in, more work incoming'), but I just couldn't get it to work. @dummdidumm has been attacking the problem as well though and has some promising results.

So I think the move is to wait for that PR, and add this test. But if it falls through, then this is definitely a good interim fix

Comment on lines +332 to +334
export function get_derived_writes() {
return derived_writes;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW we don't need to expose function wrappers to get values from different modules, only to set them. we can just export let derived_writes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always so skeptical of live exports...like under my skin I know they works but I just can't trust myself enough to do it 😂

Rich-Harris added a commit that referenced this pull request Jul 22, 2025
Rich-Harris added a commit that referenced this pull request Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Probably false positive effect_update_depth_exceeded error when reading derived firstly at effect
2 participants