Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EndpointResolverExtensions-SelectOne trashes exception stack trace #376

Closed
CornedBee opened this issue Dec 5, 2017 · 6 comments · Fixed by #377
Closed

EndpointResolverExtensions-SelectOne trashes exception stack trace #376

CornedBee opened this issue Dec 5, 2017 · 6 comments · Fixed by #377

Comments

@CornedBee
Copy link

        public static T SelectOne<T>(this IEndpointResolver resolver, Func<AmqpTcpEndpoint, T> selector)
        {
            var t = default(T);
            Exception exception = null;
            foreach(var ep in resolver.All())
            {
                try
                {
                    // ...
                }
                catch (Exception e)
                {
                    exception = e;
                }
            }

            if(Object.Equals(t, default(T)) && exception != null)
            {
                throw exception;
            }

            return t;
    }

Here, inside the loop the code captures exceptions from attempts to connect to the endpoints. It remembers the last one, and at the end, if it didn't get a successful result, it rethrows this last exception.

However, in doing so, it trashes the stack trace of the exception, making debugging the underlying problem hard or impossible. (In my particular case, I'm getting a TypeInitializerException complaining that it can't initialize Interop.Http - but RabbitMQ has, according to my decompiler, no direct or indirect dependency on System.Net.Http ...)

It should do one of two things:

Simple: It should rethrow the exception using the ExceptionDispatchInfo method:

ExceptionDispatchInfo.Capture(exception).Throw();

More complicated but better: it should capture all exceptions and wrap them in an AggregateException:

        public static T SelectOne<T>(this IEndpointResolver resolver, Func<AmqpTcpEndpoint, T> selector)
        {
            var t = default(T);
            var exceptions = new List<Exception>();
            foreach(var ep in resolver.All())
            {
                try
                {
                    // ...
                }
                catch (Exception e)
                {
                    exceptions.Add(e);
                }
            }

            if(Object.Equals(t, default(T)) && exceptions.Count > 0)
            {
                throw new AggregateException(exceptions);
            }

            return t;
    }
@michaelklishin
Copy link
Member

@CornedBee would you be interested in submitting a PR that does the latter?

@CornedBee
Copy link
Author

Sure, can do. Tip of tree is broken though due to a mismerge of pull request #368.

CornedBee pushed a commit to CornedBee/rabbitmq-dotnet-client that referenced this issue Dec 6, 2017
… and rethrow them as an AggregateException. Fixes rabbitmq#376.
@CornedBee
Copy link
Author

That PR also unbreaks the build.

@michaelklishin
Copy link
Member

@CornedBee can you please clarify what kind of "mismerge" you are referring to?

@CornedBee
Copy link
Author

CornedBee commented Dec 7, 2017

@michaelklishin At revision 2746891, client/impl/Connection.cs contains the following code:

#if CORECLR
        private static string version = typeof(Connection).GetTypeInfo().Assembly
                                                .GetCustomAttribute<AssemblyInformationalVersionAttribute>()
                                                .InformationalVersion;
#else
        private static string version = typeof(Connection).Assembly
                                            .GetCustomAttribute<AssemblyInformationalVersionAttribute>()
                                            .InformationalVersion;
#endif

#if CORECLR
        private static string version = typeof(Connection).GetTypeInfo().Assembly
                                                .GetCustomAttribute<AssemblyInformationalVersionAttribute>()
                                                .InformationalVersion;
#else
        private static string version = typeof(Connection).Assembly
                                            .GetCustomAttribute<AssemblyInformationalVersionAttribute>()
                                            .InformationalVersion;
#endif

Note that this is the same block of code repeated twice, leading to a multiple declaration error. The first of the two commits in my PR removed one of the duplicates.

I call it a mismerged because this duplication is a result of both branches being merged introducing this change, but the merge algorithm not recognizing it as the same. (This is because in one branch, it was a strict addition, while in the other, there was also a line removed just above, making it a replacement.)

@adamralph
Copy link
Contributor

It looks like CI was not run for #368:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants