Adding a yielding or erroring function to a Trove blocks every other task added to the Trove

[ambergamefam]

Right now, there is undefined behavior when adding yielding functions to a Trove:

trove:Add(function()
    print("Task #1")
end)
trove:Add(function()
    print("Task #2 started")
    task.wait(1)
    print("Task #2 completed (1 second later)")
end)
trove:Add(function()
    print("Task #3")
end)

This will block task #3 from being cleaned up until the blocking task #2 completes:

> Task #1
> Task #2 started
> Task #2 completed (1 second later)
> Task #3

While currently the workaround for this would be to wrap task #2 in a task.spawn, in a more complex environment using troves, it may be very difficult to detect these issues and debug where a yielding function is being added into a Trove.

Similarly, erroring within a trove can completely prevent the trove from cleaning up the remaining tasks:

trove:Add(function()
    if someCondition then
        error("Error!")
    end
end)

I would propose a solution (#210 ) of simply wrapping any cleanup function passed into a trove with a task.spawn:

function Trove._cleanupObject(_self: TroveInternal, object: any, cleanupMethod: string?)
    if cleanupMethod == FN_MARKER then
        task.spawn(object) -- (Previously this line of code would directly call `object()`)
    elseif cleanupMethod == THREAD_MARKER then
        pcall(task.cancel, object)
    else
        object[cleanupMethod](object)
    end
end

In the yielding example, this would output as such:

> Task #1
> Task #2
> Task #3
> Task #2 completed (1 second later)

The only disadvantage here is that calling task.spawn removes the traceback that initially caused the trove to clean up, which can be helpful to know in some scenarios.

Personally, I find the benefits of not having to debug a yield / error mistakenly added to a trove to outweigh the benefits of needing to have the traceback of why a trove cleaned up, since there are often many (tens of) Trove:Add calls for every (usually one) Trove:Destroy() / Trove:Clean() call in a codebase. It's much easier to add debug points around the code that calls Trove:Clean() than to figure out which of the many Trove:Add() calls is going wrong.

An alternative solution would be to have a task.defer thread that re-continues a Trove's cleanup if an error or yield took place in the process. However, I think this might lead to even more undefined behavior in some scenarios that would be hard to debug.

[Sleitnick]

Every other Trove-like thing I've made has used task.spawn here. Not sure why that was missed here. It should definitely be here too; I don't think Trove's cleanup should be yielding ever, nor should a cleanup stop Trove from continuing its cleanup. In terms of traceback: The error within the spawned thread will still show a good trace in terms of the function itself, so I think that's more appropriate anyway (i.e. it's the function itself that has thrown an error, and nothing to do with Trove).

Arguably, this means that the generic method-based cleanup should also be wrapped in a spawn call, but I'll leave that out for now.

[Sleitnick]

Published, Trove v1.5.0