Fix sporadical "cannot read property _values of undefined" error #7615
Conversation
| for (const property in (this: any).paint._values) { | ||
| const value = (this: any).paint.get(property); | ||
| if (!this.paint) | ||
| return false; |
There was a problem hiding this comment.
Why is paint (or layout) optional for this StyleLayer?
There was a problem hiding this comment.
Because it's calculated on recalculate, not in constructor. Alternatively, we can try setting it to empty objects there, but I think the PR solution is fine.
There was a problem hiding this comment.
Hm, I think we should investigate this further rather than covering up the symptoms. Returning false from isStateDependendent sounds wrong (since it's in an undefined state). Instead, we should not call isStateDependent when recalculate has never been called, or ensure that it is called in the constructor.
There was a problem hiding this comment.
We could investigate this further if there was a consistently reproducible test case, but since the issue is a rare race condition we only know of from production stack trace logs, I'd personally be fine with fixing the symptoms.
There was a problem hiding this comment.
we should not call isStateDependent when recalculate has never been called, or ensure that it is called in the constructor.
I wasn't able to reproduce this error condition even after a bunch of experimentation. In 36270d I took the approach of moving the paint property evaluation to the worker. This should have the additional benefit of not needing to parse the expressions on the main thread.
|
I'm currently facing a similar issue. Would love to get an update about the pending PR. |
|
@asheemmamoowala let's resurrect that fix-7523 branch into a PR maybe? |
Closes #7523. I'm not able to trace the exact race condition that causes the error, but it should be fixed with this change. Also slightly improves typing, which would catch the potential error if we didn't use the
(obj: any).propertyhack there.Launch Checklist
write tests for all new functionalitydocument any changes to public APIspost benchmark scores