-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: Disposed geometryGroup softlocks sector load #3946
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3946 +/- ##
==========================================
- Coverage 73.54% 73.52% -0.03%
==========================================
Files 357 357
Lines 35855 35870 +15
Branches 2734 2735 +1
==========================================
+ Hits 26369 26372 +3
- Misses 9381 9393 +12
Partials 105 105
|
"@reveal/utilities": "workspace:*" | ||
} | ||
} | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was changed to lf file endings, which I believe should be correct?
}, | ||
"dependencies": { | ||
"@reveal/data-providers": "workspace:*", | ||
"@reveal/logger": "workspace:*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a logger reference is the only change in this file except for line endings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
This fixes an issue where a sector would load a disposed geometry group, and then never be loadable again.
The stack trace of the issue this fixes:
I'm not actually sure why the reference imbalance happens. It seems to be easier to trigger with a small sector MemoryCache, but it may happen without any sector cache at all. It does not seem to happen if network cache is disabled. So its a probably a combination of a few factors and hard to reliably reproduce.
This PR suggests a workaround fix by not failing hard if the sector is already disposed, as this would stop all loading and the sector would never recover from the exception. Only fix for the user was a refresh of the website.
A more long term fix would be to identify the race-condition that causes a disposed sector geometry group to be loaded.
How has this been tested? 🔍
See below, I can try to add some tests if I understand what causes the issue.
Test instructions ℹ️
(In reveal Examples with a local model): Reduce budget to (1%) and navigate around a model (such as Huldra) quickly. (Move backwards / forwards / spin / anything). The log message should appear quite reliably. In real world testing this is harder to reproduce, but still happens especially with a low budget (3 - 4 million triangles budget).
Checklist ☑️