amazon-archives / aws-sdk-unity

ARCHIVED: The aws sdk for unity is now distributed as a part of aws sdk for dotnet:
https://github.com/aws/aws-sdk-net
Other
105 stars 43 forks source link

*Don't* swallow errors from the callback. Let it fail, in another th… #95

Closed seaders closed 6 years ago

seaders commented 8 years ago

…read / Coroutine, but don't swallow it. It causes all sorts of issues in real-life scenarios.

This is especially relevant for how we all work with the Amazon SDK, with anonymous lambdas, so the only sort of debugging information you get after an exception has been thrown is something like,

 An unhandled exception was thrown from the callback method, <>m__0

Swallowing errors, and displaying incomplete largely unhelpful reports of those errors is really, really, really bad practise ESPECIALLY when we're talking about swallowing errors of CALLBACK methods. Sure, you don't want to break your internal flow, that's fine. But then detach from it, through another thread, or coroutine, but DON'T swallow the errors.

karthiksaligrama commented 8 years ago

at the moment we are really trying to move away from UnityInitializer so i wouldn't add more stuff there. I'm however curious to know what logger have you set up? Cause there should be a complete stack trace before the message you mentioned above is printed.

https://github.com/aws/aws-sdk-unity/blob/master/Assets/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/UnityDebugLogger.cs#L41

seaders commented 8 years ago

Really?

Just attach,

using UnityEngine;

using Amazon;
using Amazon.CognitoIdentity;

public class Main : MonoBehaviour
{
    void Start ()
    {
        CognitoAWSCredentials credentials = new CognitoAWSCredentials("1234abcd", RegionEndpoint.USEast1);
        credentials.GetCredentialsAsync( result => {
            Debug.Log(result);
            Debug.Log(result.Exception.Data);
            Debug.Log(result.Response.AccessKey);
        } );
    }
}

To a script, and run it.

No matter if it's successful (result.Exception will be null, so result.Exception.Data will cause an exception), or it fails (result.Response will be null, so result.Response.AccessKey will cause an exception), an exception occurs, and you get the completely ambiguous, unhelpful,

System.Collections.Hashtable
UnityEngine.Debug:Internal_Log(Int32, String, Object)
UnityEngine.Debug:Log(Object)
<Start>c__AnonStorey0:<>m__0(AmazonCognitoIdentityResult`1) (at Assets/Main.cs:13)
Amazon.CognitoIdentity.<ExecuteAsync>c__AnonStorey1D`1:<>m__4E() (at Assets/Plugins/AWSSDK/src/Services/CognitoIdentity/Custom/_unity/CognitoIdentityAsyncExecutor.cs:55)
Amazon.Runtime.Internal.UnityMainThreadDispatcher:ProcessRequests() (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Pipeline/_unity/UnityMainThreadDispatcher.cs:88)

(Filename: Assets/Main.cs Line: 13)

Object reference not set to an instance of an object
UnityEngine.Debug:Internal_Log(Int32, String, Object)
UnityEngine.Debug:LogError(Object)
Amazon.Runtime.Internal.Util.UnityDebugLogger:Error(Exception, String, Object[]) (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/_unity/UnityDebugLogger.cs:48)
Amazon.Runtime.Internal.Util.Logger:Error(Exception, String, Object[]) (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/Logger.cs:155)
Amazon.Runtime.Internal.UnityMainThreadDispatcher:ProcessRequests() (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Pipeline/_unity/UnityMainThreadDispatcher.cs:94)

(Filename: Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/_unity/UnityDebugLogger.cs Line: 48)

An unhandled exception was thrown from the callback method
UnityEngine.Debug:Internal_Log(Int32, String, Object)
UnityEngine.Debug:LogError(Object)
Amazon.Runtime.Internal.Util.UnityDebugLogger:Error(Exception, String, Object[]) (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/_unity/UnityDebugLogger.cs:51)
Amazon.Runtime.Internal.Util.Logger:Error(Exception, String, Object[]) (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/Logger.cs:155)
Amazon.Runtime.Internal.UnityMainThreadDispatcher:ProcessRequests() (at Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Pipeline/_unity/UnityMainThreadDispatcher.cs:94)

(Filename: Assets/Plugins/AWSSDK/src/Core/Amazon.Runtime/Internal/Util/_unity/UnityDebugLogger.cs Line: 51)

Nonetheless, that's still not the point. Handling exceptions all you want within the SDK, but that callback is our game code, there's absolutely no good reason at all, and it's a really bad practise for internal SDK code to be swallowing errors that may happen outside of that domain.

Sure, you want to ensure that an exception doesn't disrupt the flow of the internal workings of the SDK, but then detach from it, via a CoRoutine, some other thread, but running code outside of your domain in a try/catch, and you taking it over, obfuscating it... however well you perceive you're logging it, it should never happen.

seaders commented 8 years ago

Actually the one above is being caused by UnityMainThreadDispatcher.cs, where there's a multitude of the same problem, try / catch around callbacks. Again to re-iterate, when you're passing control back to code outside of your domain, you should never be swallowing the exceptions.

hyandell commented 6 years ago

Closing per Karthik's comment.