AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 288 forks source link

#658 Fix for exception during OnShutdown causing other OnShutdowns to be skipped #662

Closed Barsonax closed 6 years ago

Barsonax commented 6 years ago

Not yet finished and this PR is mostly for discussion of issue #658 currently.

Possible improvements:

deanljohnson commented 6 years ago

For the ShutdownAllReversed method - couldn't the call site just do list.Reverse().ShutdownAll? This is already done in a few places before ShutdownAll is called so there is a precedence for it. None of this code is in a hot path either so I don't think we need to worry about the bit of a performance loss.

Barsonax commented 6 years ago

Good point I also think there might be a good possibility that this will be used for more than just shutdown (also for init, update etc?). So eventually I want to generalize the code alot more. The current implementation is more like a proof of concept to play with.

Before making the current implementation better Iam interested in what @ilexp's opinion is about this topic. Maybe he had very good reasons for not doing it like this.

deanljohnson commented 6 years ago

Well if we wanted to generalize it with the minimum amount of code duplication we could create a TryForAll(this IEnumerable<object> objects, Action<object> action). It would iterate anything and perform the given actions in a try/catch and produce a AggregateException - maybe as a return value? TryForAll could probably be generic as well. Just an idea.

Your throw line could also check if there is only one exception in the list and throw that exception specifically if that's the case. This would be more intuitive for the user I think.

Barsonax commented 6 years ago

Well if we wanted to generalize it with the minimum amount of code duplication we could create a TryForAll(this IEnumerable<object> objects, Action<object> action). It would iterate anything and perform the given actions in a try/catch and produce a AggregateException - maybe as a return value? TryForAll could probably be generic as well. Just an idea.

Ye I was thinking something along this line as well:

[DebuggerStepThrough]
public static bool TryExecute<TObject>(this IEnumerable<TObject> objects, Action<TObject> action, out IList<Exception> exceptions)
{
    exceptions = null;
    foreach (var o in objects)
    {
        try
        {
            action.Invoke(o);
        }
        catch (Exception e)
        {
            if (exceptions == null) exceptions = new List<Exception>();
            exceptions.Add(e);
        }
    }

    return exceptions != null;
}

throw line could also check if there is only one exception in the list and throw that exception specifically if that's the case. This would be more intuitive for the user I think.

I think that might be better handled higher up in the application by logging the individual errors inside the aggregrate exception as that wont hide any errors inside the aggregrate exception even if there are dozens (possibly with a upper limit to prevent freezing the app?).

ilexp commented 6 years ago

Well if we wanted to generalize it with the minimum amount of code duplication we could create a TryForAll(this IEnumerable objects, Action action). It would iterate anything and perform the given actions in a try/catch and produce a AggregateException - maybe as a return value? TryForAll could probably be generic as well. Just an idea.

Great idea from a design perspective, but not sure what it would do to performance, as passing-by-delegate potentially produces garbage and cannot be optimized as thoroughly as a regular function call - I'd generally try to avoid this in spots like this. If having specialized methods instead has a big impact design / maintenance wise though, we should probably consider it anyway. It's a tradeoff.

Before making the current implementation better Iam interested in what @ilexp's opinion is about this topic. Maybe he had very good reasons for not doing it like this.

Performance considerations mostly, as a try-catch block isn't free if I recall correctly, especially when optimizations (or lack thereof) come into play. Would still be willing to consider making the change, as you're right that there's somewhat severe editor / dev bug potential when left entirely unchecked.


That said, this is a change that should be done based on the v3 branch, as a lot of the related code has changed, especially due to the ICmpInitializable split. Closing this PR, but would welcome a v3 implementation for review and gradual iteration.