Skip to content
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

Hang introduced by #5269, (a0f93cb) (fix included) #6646

Closed
sarahsvedenborg opened this issue May 14, 2024 · 3 comments · Fixed by #6699
Closed

Hang introduced by #5269, (a0f93cb) (fix included) #6646

sarahsvedenborg opened this issue May 14, 2024 · 3 comments · Fixed by #6699
Assignees

Comments

@sarahsvedenborg
Copy link
Contributor

sarahsvedenborg commented May 14, 2024

If you find a security vulnerability, do NOT open an issue. Email security@sanity.io instead.

Describe the bug
After upgrading from 3.19.2 to 3.41.1, we experience a consistent hang when navigating between documents in sanity studio. The document seems to pend loading forever and ends up with the browser (Chrome 124.0.6367.202 Mac OS) forcing the user to close the tab.

As you would expect, this only occurs for some of our schemas and we have been unable to isolate exactly which part of said schemas.

After digging into the code this behaviour was introduced by #5269 a0f93cb, more specifically the changes to _immutableReconcile.

The change to _immutableReconcile introduced in #5269 appears to try to fix the issue that visited subtrees will not be identically reconciled by removing them from the visited parents. However, this also means that the memoization-effect from skipping visited subtrees is now gone.

The effect on our studio is that the reconciliation takes a VERY long time, and that the tab hangs. I believe this is due to exponential recursion introduced by #5269 and will only be noticeable for sufficiently large objects.

To Reproduce
We have been unable to isolate a sufficiently small input-case, but have been able to produce a patch that fixes the issue, see below.

Expected behavior
No hang.

Screenshots
If applicable, add screenshots to help explain your problem.

Which versions of Sanity are you using?

@sanity/cli (global) 3.41.1 (up to date)
@sanity/cli 3.41.1 (up to date)
@sanity/components 2.14.0 (up to date)
@sanity/form-builder 2.36.2 (up to date)
@sanity/schema 3.41.1 (up to date)
@sanity/types 3.41.1 (up to date)
@sanity/ui 2.1.6 (latest: 2.1.7)
@sanity/vision 3.41.1 (up to date)
sanity 3.41.1 (up to date)

What operating system are you using?
MAC OS

Which versions of Node.js / npm are you running?

Run npm -v && node -v in the terminal and copy-paste the result here.
10.2.0
v21.1.0

Additional context

Add any other context about the problem here.

Security issue?

No

Patch

Changing the "parents"-set in immutableReconcile to a map that remembers the reconciliation for each sub-tree ensures we do not re-process a subtree more than necessary, in addition to ensuring that subsequent visits to the same subtree are properly reconciled. (Ref #5269).

/**
 * Reconciles two versions of a state tree by iterating over the next and deep comparing against the next towards the previous.
 * Wherever identical values are found, the previous value is kept, preserving object identities for arrays and objects where possible
 * @param previous - the previous value
 * @param next - the next/current value
 */
export function immutableReconcile<T>(previous: unknown, next: T): T {
  return _immutableReconcile(previous, next, new WeakMap())
}

function _immutableReconcile<T>(
  previous: unknown,
  next: T,
  /**
   * Keep track of visited nodes to prevent infinite recursion in case of circular structures
   */
  parents: WeakMap<any, any>,
): T {
  if (previous === next) return previous as T

  if (parents.has(next)) {
    return parents.get(next);
  }

  // eslint-disable-next-line no-eq-null
  if (previous == null || next == null) return next

  const prevType = typeof previous
  const nextType = typeof next

  // Different types
  if (prevType !== nextType) return next

  if (Array.isArray(next)) {
    assertType<unknown[]>(previous)
    assertType<unknown[]>(next)

    let allEqual = previous.length === next.length
    const result = []
    parents.set(next, result)
    for (let index = 0; index < next.length; index++) {
      const nextItem = _immutableReconcile(previous[index], next[index], parents)

      if (nextItem !== previous[index]) {
        allEqual = false
      }
      result[index] = nextItem
    }
    parents.set(next, allEqual ? previous : result)
    return (allEqual ? previous : result) as any
  }

  if (typeof next === 'object') {

    assertType<Record<string, unknown>>(previous)
    assertType<Record<string, unknown>>(next)

    const nextKeys = Object.keys(next)
    let allEqual = Object.keys(previous).length === nextKeys.length
    const result: Record<string, unknown> = {}
    parents.set(next, result)
    for (const key of nextKeys) {
      const nextValue = _immutableReconcile(previous[key], next[key]!, parents)
      if (nextValue !== previous[key]) {
        allEqual = false
      }
      result[key] = nextValue
    }
    parents.set(next, allEqual ? previous : result)
    return (allEqual ? previous : result) as T
  }
  return next
}

// just some typescript trickery get type assertion
// eslint-disable-next-line @typescript-eslint/no-empty-function, no-empty-function
function assertType<T>(value: unknown): asserts value is T {}
@bjoerge
Copy link
Member

bjoerge commented May 15, 2024

Hi, thanks for the thorough investigation and the proposed fix @sarahsvedenborg! Unfortunately your proposed fix breaks one of our unit tests, so we need to spend a bit more time on understanding why this happens for you. This is the first time I hear about this beging an issue, so suspect it might be something about your particular setup that triggers this.

I understand that it's difficult to create a minimal reproducible case, but any more details about your schema/setup would be of great help and also allows us to capture your case in a unit test to make sure we don't regress. Feel free to relay any details through support channels.

bjoerge added a commit that referenced this issue May 15, 2024
@stian-svedenborg
Copy link
Contributor

@sarahsvedenborg is working on producing a minimal test-case, but I have a question regarding the failing unit test.

Are you sure the unit test is working as intended?

The proposed fix has one functional difference to the existing reconcile, namely that it continues to reconcile objects within loops, the old version does not and will give up on reconciling objects or arrays encountered in the second iteration of a loop and simply pick "next" even if subobjects of next could still be reconciled, could this be the reason for the failing unit test?

To illustrate:

const objectCreator = (differentiator) => {
  const root = { id: "root" } // will be different if differentiator is different
  const a = { id: "a" }   // will be different if differentiator is different
  const b = { id: "b", diff: differentiator }  // will be different if differentiator is different
  const c = { id: "c" }  // will never be different

  root.a = a;
  root.a.b = b
  root.a.b.a = a; // loop
  root.a.b.c = c;

  return root
}

const previous = objectCreator("previous")
const next = objectCreator("next")
const reconciledOld = immutableReconcileOld(previous, next)
const reconciledNew = immutableReconcileNew(previous, next)

// Current Behaviour
assertNotSame(previous, reconciledOld)
assertNotSame(next, reconciledOld)
assertSame(previous.a.b.c, reconciledOld.a.b.c)
assertNotSame(previous.a.b.a.b.c, reconciledOld.a.b.a.b.c) // (Suboptimal) Equal objects nested within loops are not retained in the second iteration of the loop.
assertNotSame(reconciledOld.a, reconciledOld.a.b.a) // Loops inside reconciled objects are not retained. 
assertSame(next.a.b.c, reconciledOld.a.b.a.b.c) // This is because the old reconcile always picks "next" when hitting a node that it is already resolving.

// Proposed behaviour
assertNotSame(previous, reconciledNew)
assertNotSame(next, reconciledNew)
assertNotSame(next.a, reconciledNew.a) // A sub object of root has changed, creating new object
assertNotSame(next.a.b, reconciledNew.a.b) // A sub-object of root.a has changed, creating new object
assertNotSame(next.a.b.c, reconciledNew.a.b.c) // root.a.b.c is has not changed, therefore reuse.
assertSame(previous.a.b.c, reconciledNew.a.b.c)   

assertSame(previous.a.b.a.b.c, reconciledNew.a.b.a.b.c) // The new reconcile will retain reconcilable objects also within loops.
assertSame(reconciledNew.a, reconciledNew.a.b.a)  // This is because it retains the loop.
assertSame(previous.a.b.c, reconciledNew.a.b.a.b.c)

If this is the reason for the failing unit-test I would propose changing the test, as the new reconcile will be both faster and reconcile more objects.

bjoerge added a commit that referenced this issue May 16, 2024
@bjoerge
Copy link
Member

bjoerge commented May 16, 2024

Great observations @stian-svedenborg – Think you're absolutely right here. I'll set aside some time today to integrate your proposed unit test into our test suite and make a PR based on @sarahsvedenborg's fix. Thanks again for digging into this.

bjoerge added a commit that referenced this issue May 16, 2024
bjoerge added a commit that referenced this issue May 16, 2024
bjoerge added a commit that referenced this issue May 16, 2024
bjoerge added a commit that referenced this issue May 16, 2024
bjoerge added a commit that referenced this issue May 27, 2024
Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>
github-merge-queue bot pushed a commit that referenced this issue May 27, 2024
…inite loop (#6699)

* test(form): improve test for immutableReconcile handling cyclic structures

Co-authored-by: @stian-svedenborg <deinoff@gmail.com>

* fix(sanity): add fix proposed in #6646

Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>

---------

Co-authored-by: @stian-svedenborg <deinoff@gmail.com>
Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>
@bjoerge bjoerge reopened this May 27, 2024
@bjoerge bjoerge closed this as completed May 27, 2024
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 a pull request may close this issue.

3 participants