-
Notifications
You must be signed in to change notification settings - Fork 409
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
Comments
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. |
@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:
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. |
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. |
Co-authored-by: @sarahsvedenborg <sarah.svedenborg@gmail.com>
…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>
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).
The text was updated successfully, but these errors were encountered: