Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### 🐞 Bug fixes
- Fix updating terrain tiles on feature state changes ([#6231](https://github.com/maplibre/maplibre-gl-js/issues/6231)) (by [pstaszek](https://github.com/pstaszek))
- _...Add new stuff here..._
- fix error during task execution blocking any further task execution ([#7031](https://github.com/maplibre/maplibre-gl-js/pull/7031))

## 5.17.0

Expand Down
11 changes: 11 additions & 0 deletions src/util/task_queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,15 @@ describe('TaskQueue', () => {
q.run();
expect(after).not.toHaveBeenCalled();
});

test('Resets _currentlyRunning when a task throws an error', () => {
const q = new TaskQueue();
q.add(() => { throw new Error('Task error'); });
expect(() => q.run()).toThrow('Task error');
// Should be able to run again - _currentlyRunning should be false
const afterError = vi.fn();
q.add(afterError);
q.run();
expect(afterError).toHaveBeenCalledTimes(1);
});
});
16 changes: 9 additions & 7 deletions src/util/task_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,16 @@ export class TaskQueue {
// on the next run, not the current run.
this._queue = [];

for (const task of queue) {
if (task.cancelled) continue;
task.callback(timeStamp);
if (this._cleared) break;
try {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the task execution be wrapped in try catch and not the entire for loop?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this ensures that exiting the function always releases the _currentlyRunning lock

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What will happen if I add two tasks - the first that throws and the second that should pass.
And then call the run method? Will the second task run or be "ignored"?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Don't know. Don't have any idea how this Task system works, my boss just complained about this error so I found the linked issue and did a PR for the solution they suggested and never bothered to PR. Hopefully someone who knows anything about this Task system can check it out

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would recommend trying to take a deeper look then, otherwise I'll have a hard time merging this as is...

for (const task of queue) {
if (task.cancelled) continue;
task.callback(timeStamp);
if (this._cleared) break;
}
} finally {
this._cleared = false;
this._currentlyRunning = false;
}

this._cleared = false;
this._currentlyRunning = false;
}

clear() {
Expand Down