fix over-eager deletion of task groups on trap

[dicej]

Instead of deleting any outstanding task groups when trapping, we now just record that we've notified the task hook so we don't do it again if and when the group's ref count goes to zero.

Fixes #14504

[alexcrichton]

I'm a bit confused by how this works -- the finished field is never set to true while the groups are in the table, except when the store is torn down. Was this a problem where during teardown the same group was deleted twice? If so could that be solved by reordering some teardown?

[dicej]

I'm a bit confused by how this works -- the finished field is never set to true while the groups are in the table, except when the store is torn down. Was this a problem where during teardown the same group was deleted twice? If so could that be solved by reordering some teardown?

clean_up_task_groups is called in two places: when a store is poisoned due to a trap and when it is disposed. In either case, the may be other cleanup after that call completes (e.g. gracefully disposing of fibers, etc.) which may cause guest and/or host tasks to be disposed, in which case we may try to look up the task group for those tasks to dispose it, but since clean_up_task_groups had already deleted the group, we errored, and that error was escalated to a panic in SignalOnDrop::drop.

[alexcrichton]

Do we need to run this on set_trapped? Would it be possible to only run this on store teardown?

[dicej]

Do we need to run this on set_trapped? Would it be possible to only run this on store teardown?

No, we don't have to; I figured it would be best to do it promptly, but I'd be fine with only doing it on store teardown. And note that we'd have to make sure it's pretty much the last thing we do on store teardown if we want to delete any task groups before their reference counts go to zero; otherwise, we'll risk hitting this issue again.

[alexcrichton]

I think that'd be best to implement yeah, I found it pretty surprising that set_trapped also had a side effect of running arbitrary embedder code

[dicej]

@alexcrichton I've applied your feedback and rebased onto main.