Skip to content

Fix redoPlacement race condition - #5185

Merged
mourner merged 1 commit into
masterfrom
fix-redo-placement-error
Aug 28, 2017
Merged

mourner merged 1 commit into
masterfrom
fix-redo-placement-error

Conversation

@mourner

@mourner mourner commented Aug 23, 2017

Copy link
Copy Markdown
Member

Closes #3700. The condition occurred when a tile requested redoPlacement (which sent a redoPlacement request to the worker) and then shortly got removed (before getting response to redoPlacement back from the worker) — stopPlacementThrottler did not prevent redoPlacement from running in this case because it was past the throttling phase.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

@mourner
mourner requested a review from ChrisLoer August 23, 2017 14:29
@mourner
mourner force-pushed the fix-redo-placement-error branch from d1b1bb5 to a4b7243 Compare August 23, 2017 14:55
@mourner mourner mentioned this pull request Aug 23, 2017
2 of 3 tasks
@mourner

mourner commented Aug 23, 2017

Copy link
Copy Markdown
Member Author

Not sure if it's worth adding a test for this tricky condition if the placement code is fully rewritten in the viewport-collision branch anyway.

@ChrisLoer

Copy link
Copy Markdown
Contributor

I think this change would turn the redoWhenDone functionality into a no-op. Looking back even before the throttling changes went in, the behavior used to be that the tile state would be 'loaded', but another placement request would happen. I think that was a bug -- the state should have always been switched to 'reloading'.

Did the throttling changes cause #3700, or did they just change the timing so that it was easier to expose this condition?

@mourner
mourner force-pushed the fix-redo-placement-error branch from a4b7243 to 80d3d89 Compare August 28, 2017 08:46
@mourner

mourner commented Aug 28, 2017

Copy link
Copy Markdown
Member Author

@ChrisLoer fixed it to add the reloading state on redoWhenDone. If you think we'll do one more release before the viewport labeling PR is finalized, this is worth merging, otherwise we should close to reduce merge collisions. Not sure what caused the regression in the first place but throttling could be related.

@ChrisLoer ChrisLoer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think it's worth merging.

@mourner
mourner merged commit f8a0b0f into master Aug 28, 2017
@mourner
mourner deleted the fix-redo-placement-error branch August 28, 2017 15:41
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 this pull request may close these issues.

2 participants