Conversation
Cancel the queued task and settle the layer update on finalization without terminating a shared pool. Ignore running tasks after finalization and remove abort listeners when updates settle. Await each task instead of pool.completed() so unrelated work cannot delay or fail the update. Race the task against abort because threads does not settle cancelled tasks. Closes geoarrow#227
Check that queued Polygon and MultiPolygon tasks are cancelled and their updates settle before a busy shared worker is released. Verify that the pool remains usable. Cover running tasks, late failures, owned-pool shutdown, listener cleanup, and finalization during initialization or before a state update.
|
Maintainers - please let me know if reviewing a combined PR that brings changes from both #230 and this would be easier to review. Happy to make a new PR if that's the case! |
|
I don't know which of this and #230 I should review first if they touch the same code? |
|
I see the confusion @kylebarron . They issues they adress are separate, but fixes lay in the same files. I'd focus on the issues first. If you agree that issue #225 is something we should fix (moving prep work into the queue), then lets focus on #230 first. What I tried to open up for in my last comment is that if you also agree we should address issue #227 I could create a joint PR for both to save some review-cycles. Ideally I would have used the new "stacked PRs" but github does not allow that feature from forks. PR #232 (this PR) was created to provide a solution for issue #227 if issue #225 was discarded. |
2 participants
Closes #227
Note: this touches some of the same files as PR #230. It does independently add value to this repository, but the combined changes in both PR would bring the most (where #230 would also additionally avoid copying geometry for tasks cancelled before they start). I expect gihub to flag for merge conflicts if that PR merges before this one. Regardless this PR needs to adapt if PR 230 merges before it (see below).
This PR improves the handling of finalizeState to reduce unnecessary work. Removes queued triangulation work, prevents running triangulation work results from updating "finalized" layers, cancels / finishes pending updates without waiting for busy workers and cleans up cancellation listeners.
In my project this is important to improve performance when users are panning the map.
Tests cover Polygon and MultiPolygon cancellation before a busy worker is released, shared-pool reuse, running tasks and late failures, listener cleanup, and finalization during initialization or before a state update.
LLM summarised the changes needed if PR #230 is already merged as:
Keep #230's geometry preparation inside the queued callback. In both
_earcutPolygonData()and_earcutMultiPolygonData(), pass that callback to_queueEarcutTask(pool, callback)instead of callingpool.queue(callback)directly.Keep this PR's finalization guards, abort handling, listener cleanup, and
try/finallymetrics cleanup. Awaiting the task alone is insufficient: threads does not settle cancelled tasks, so the race against the abort signal is still needed.The current revisions produce one conflict block in each triangulation method. Check the surrounding code too: Git automatically retains the direct
pool.queue()call outside the conflict markers. It must be replaced, not left alongside_queueEarcutTask().The test files are separate and should both be retained.