Exception during the ICmpInitializable.OnShutdown event skips all remaining OnShutdowns

[Barsonax]

Summary

A Exception during the ICmpInitializable.OnShutdown event will skip any ICmpInitializable.OnShutdown after that. Looking at the source code here its easy to see why since there is no try catch to prevent this from happening. This can cause all kinds of weird bugs such as events not being unsubscribed.

A solution would be to collect all exceptions and throw/log them after all Shutdowns have executed.

Consider applying this to all events.

How to reproduce

• Download ShutdownSkipped.zip
• Open and run it in the editor. Note that even though we have 2 components only 1 of them has its shutdown executed

Workaround

• Prevent any Exceptions during this event.

[ilexp]

There are no editor guards in place in order to prevent an exception from inside user code getters or setters to propagate into the editor - however, I think it should be possible to add try / catch clauses like that in spots where these kinds of errors are most likely to occur.

Is there a callstack we can work with? From where exactly is that getter called when the problem occurs?

[Barsonax]

So I tried the following things: -Make the property invisible in the UI. -Change the property to a method.

Neither of those changes seem to solve the problem but it doesnt seem to be caused by the UI (directly, maybe still indirect?. Restarting duality does fix the issue.

The code is used in a DualityApp.Mouse.ButtonDown event. Iam still in the process of isolating this completely so sorry for the current lack of info.

EDIT: here is a test project to easily reproduce this error. Instructions can be found at the top of the issue: NullreferenceBugInEditorResearch.zip)

[Barsonax]

Ah seems I have forgotten to unsubscribe the DualityApp.Mouse.ButtonDown event. This in addition to the fact that new GameObjects/Components are created when you enter/leave sandbox mode means you will be calling methods on objects that are supposed to be out of scope. So this was really a (rather stupid :P) bug caused by myself.

Aside from that fact though it would be nice if the editor actually told you forgot to unsubscribe some event.

EDIT: seems that was the case in the test project I uploaded here but not in pathfindax. EDIT2: seems to be caused by the fact that one of my ICmpInitializable.OnShutdown had a exception in it. This prevented the execution of all ICmpInitializable.OnShutdown after that. Looking at the duality source its easy to see why. There is no try catch around the shutdownlist so it just skips the rest of the list once a exception has occured. EDIT3: updated the first comment to reflect the above observations. Maybe this issue needs to be splitted in 2 later on.

[ilexp]

Let's refocus this issue towards guarding against forgotten public event subscriptions then 👍

Can you rename / edit the issue to reflect this? Don't remove the old description and repro sample though, doesn't hurt to have them still around, maybe just "quote" them at the bottom as original text.

[Barsonax]

So a solution to this would follow something like the following piece of code:

var list = new List<Exception>();
for (int i = 0; i < 10; i++)
{
    try
    {
        SomeShutdownMethod();
    }
    catch (Exception e)
    {
        list.Add(e);
    }
}

if (list.Count > 0)
{                
    throw new AggregateException(list); //Throw/log the exception here
}

However this has a downside since the debugger will no longer automatically break at the original exception/exceptions. This can be solved with the [DebuggerStepThrough] attribute though. This attribute will cause the debugger to break at the original location of the exception even though we are catching it.

[Barsonax]

Moved the Editor doesnt tell you you forgot to unsubcribe a event issue to a separate issue #666

[Barsonax]

Replying to @ilexp comment on the #662 PR

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.

A delegate invocation does not produce garbage if it does not capture a local variable. Aside from that a delegate invocation is pretty just as fast as a interface call. I doubt this will lead to noticable performance issues or that the difference is even measurable in duality.

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.

In the happy path I don't think there is any noticable difference with a try catch block. Only when actual exceptions are thrown there will be a difference. So I don't think we should be afraid of putting try catch blocks where its needed.

But like always with performance only by measuring it you will know it for sure.

[ilexp]

A delegate invocation does not produce garbage if it does not capture a local variable.

Delegate invocation might not, but delegate creation does. Unless you cache the implicitly created delegate when passing some method to the delegate parameter, you'll create a new one every time.

Aside from that a delegate invocation is pretty just as fast as a interface call. I doubt this will lead to noticable performance issues or that the difference is even measurable in duality.

Even if the invocation was free, it's still less compiler / JIT optimizable than having a specific method called.

You're probably right on this for init and shutdown though, just let's not expand this to update and render calls, or expect one generalized method to cover them all via delegate.

In the happy path I don't think there is any noticable difference with a try catch block. Only when actual exceptions are thrown there will be a difference. So I don't think we should be afraid of putting try catch blocks where its needed.

try..catch will block some compiler optimizations, so I wouldn't just put them wherever.

My point in both cases is just that we're in territory where we should spend some thought on how a design change might impact performance and compiler optimizations. Handling components - that's a big part of what the update cycle does all the time. Compiler friendly code can pay off when dealing with 10k objects potentially every frame. It's probably worth the tradeoff for init and shutdown, just a general "beware" sign here 🙂

[ilexp]

Potentially related: Issue #381

[Barsonax]

I think it might be worth benchmarking the code to see what really happens.

My current understanding is that lambdas's (statically typed and without closures) do not have any GC overhead. It gets optimized away. This might be proved wrong though. On top of that I believe their invocation to be equally fast as a interface/virtual call.

Try catch will increase the size of the method body I believe so it could potentially affect performance. I suspect this difference to be on the single nanosecond scale or maybe less though.

Will report back when I got the results.