Clean up abort algorithms and task queue map entries #84
Conversation
| queue/removal steps=]. | ||
| 1. Set |handle|'s [=task handle/task complete steps=] to the following steps: | ||
| 1. If |signal| is not null, then [=AbortSignal/remove=] |handle|'s [=task handle/abort steps=] | ||
| from |signal|. |
There was a problem hiding this comment.
Nevermind, I see now, once we complete this handle we don't need the signal to call the abort steps any more.
There was a problem hiding this comment.
That's right. I need to fix this in some other specs too, but abort algorithms can accumulate, which can be leaky.
|
|
||
| 1. Let |queue| be a new [=scheduler task queue=]. | ||
| 1. Set |queue|'s [=scheduler task queue/priority=] to |priority|. | ||
| 1. Set |queue|'s [=scheduler task queue/tasks=] to a new empty [=set=]. | ||
| 1. Set |queue|'s [=scheduler task queue/removal steps=] to |removalSteps|. |
There was a problem hiding this comment.
A new task queue starts empty and has removal steps attached.
After the last task is rejected/completed, we will call the removal steps, and those steps I think make this queue no longer valid without re-creating.
I don't think it can happen per the changes in this change, but it seems like a task-queue must never stay empty after creation, nor ever return back from non-empty?
There was a problem hiding this comment.
I think this comment refers to the risk of having a reference to the TQ but the queue having been removed from the map? That would indeed be a problem -- and I did have to rewrite things to make sure that can't happen (specifically in the delayed task section, which previously got the TQ when postTask called, but didn't actually queue the task until the delay expired).
A task queue could go from empty to non-empty in the same task, e.g. the last task is dequeued, runs, queues more tasks to the queue within that task. But in that case it's okay because it would still be in the map.
Implementations might want to do the cleanup differently, e.g. in Chrome we use a weak map from signal --> task queue and let GC take care of cleanup (similar for abort algorithms -- we have a GCed handle). But this isn't observable since the task queues themselves aren't web-exposed. Adding the cleanup explicitly here at least makes it so the spec isn't leaky, and indicates when resources could be released.
This introduces removal of abort algorithms and task queue map entries
on abort and task completion:
- Add a helper struct (task handle) to store state and algorithms
needed to clean up on abort/task complete. It was a bit cleaner to
keep this all together.
- On abort and task complete, remove the task queue map entry if the
queue became empty
- On task complete, remove the abort algorithm
- On abort, remove the pending task
Notes:
1. This also fixes a spec bug where it was possible for delayed tasks
not to be aborted if the abort happened while waiting to queue the
task. This is fixed here by moving up the point where we add the
abort algorithm and conditionally and adjusting the algorithm to
handle the case where the task hasn't been queued yet (during
delay).
2. For delayed tasks, getting the destination task queue is deferred
until actually queuing the task (after the delay expires). It's
important to wait to ensure the map entry exists when queuing the
task.
3. Chromium removes both abort algorithms and task queues lazily
during GC, which is another valid approach (these are not web
observable).
shaseley
force-pushed
the
clean-up-state
branch
from
b3b6a19 to
e855dee
Compare
This introduces removal of abort algorithms and task queue map entries on abort and task completion:
Notes:
This also fixes a spec bug where it was possible for delayed tasks not to be aborted if the abort happened while waiting to queue the task. This is fixed here by moving up the point where we add the abort algorithm and conditionally and adjusting the algorithm to handle the case where the task hasn't been queued yet (during delay).
For delayed tasks, getting the destination task queue is deferred until actually queuing the task (after the delay expires). It's important to wait to ensure the map entry exists when queuing the task.
Chromium removes both abort algorithms and task queues lazily during GC, which is another valid approach (these are not web observable).
Preview | Diff