Fix race condition in Tile redoPlacement #3985
Conversation
| @@ -129,11 +129,11 @@ class Tile { | |||
| } | |||
|
|
|||
| redoPlacement(source) { | |||
| if (source.type !== 'vector' && source.type !== 'geojson') { | |||
| if ((source.type !== 'vector' && source.type !== 'geojson') || | |||
| !this.hasData() || !this.collisionTile) { | |||
There was a problem hiding this comment.
Does this affect the behavior if redoPlacement is called before the tile is initially loaded? For example, if the map is rotated while tiles are loading, it may be necessary to redo placement immediately after the load completes. This normally happens via this.redoWhenDone, but it appears that this change may bypass that logic.
There was a problem hiding this comment.
@jfirebaugh I thought about that, but the problem is that this logic doesn't work currently anyway, because redoWhenDone is only processed in the done callback which can only happen after a successful redoPlacement request (after the tile got loaded). Maybe we should rewrite the code to take this case into account in a follow-up PR.
There was a problem hiding this comment.
redoWhenDone is also used when the tile is loaded, here.
There was a problem hiding this comment.
Ah, I missed that somehow. Will fix, thanks for spotting this.
There was a problem hiding this comment.
Fixed now. Ready for review.
There was a problem hiding this comment.
Can you explain how this avoids the issue? It looks like checking !this.collisionTile will still prevent reaching the this.redoWhenDone = true; case when redoPlacement is called while the tile is initially loading.
There was a problem hiding this comment.
Looks like I'm having a bad day. Amended the commit, need to add a test for this.
There was a problem hiding this comment.
@jfirebaugh added a few more basic tests for this.
mourner
force-pushed
the
fix-reload-symbol-error
branch
from
f8b5c9e to
2d4e854
Compare
mourner
force-pushed
the
fix-reload-symbol-error
branch
from
2d4e854 to
5763249
Compare
| @@ -129,14 +129,16 @@ class Tile { | |||
| } | |||
|
|
|||
| redoPlacement(source) { | |||
| if (source.type !== 'vector' && source.type !== 'geojson') { | |||
| if ((source.type !== 'vector' && source.type !== 'geojson')) { | |||
There was a problem hiding this comment.
Nit: unnecessary extra parentheses.
Closes #3700. @sguignot please confirm that this fixes the issue for you.
The race condition happened when
redoPlacementwas called for an empty GeoJSON tile. In this case tilestateisloaded, but all the data variables arenull, causingredoPlacementto proceed sending the request to redo placement when it's not necessary, and breaking in the callback. Added a basic tests to make sure this doesn't regress.Launch Checklist
document any changes to public APIspost benchmark scores