Collect all exceptions when selecting an endpoint

[CornedBee]

Proposed Changes

This fixes #376 in the second way proposed there. It not only preserves the stack trace of the caught exception, but collects all exceptions caught while trying out endpoints, and throws them all in an AggregateException if no suitable endpoint can be found.

Note that this is a user-visible behavior change that might break some edge cases: the InnerException of the BrokerUnreachableException is now an AggregateException instead of the last exception thrown from some endpoint (such as a Client.Exceptions.ConnectFailureException).

Types of Changes

• Bugfix (non-breaking change which fixes issue EndpointResolverExtensions-SelectOne trashes exception stack trace #376)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

I have run the tests with a running RabbitMQ, but not within the full umbrella test suite, so everything that needs rabbitmqctl failed locally.

[pivotal-issuemaster]

@CornedBee Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@CornedBee Thank you for signing the Contributor License Agreement!

[kjnilsson]

This looks ok to me. I'm not sure if changing the type of an inner exception qualifies as a breaking change as nowhere in the API are we actually documenting which inner exceptions form part of the API. So by that extension and AggregateException is fine. It's a SemVer grey area.

[bording]

@kjnilsson As long as there is an inheritance hierarchy between the types, going from less specific to more specific is okay, but going from more specific to less specific would be a break.

Exception -> AggregateException OK
AggregateException -> Exception BREAKING CHANGE

[bording]

I've only looked at the actual changes showing up in the PR diff and not elsewhere in the code, but I assume that the exception throw by SelectOne is caught elsewhere and then wrapped in BrokerUnreachableException and rethrown before the enduser sees it?

[kjnilsson]

@bording exactly at the type level it is non-breaking, however I still think Liskov would have a case here. It would be possible that some users may have logic hanging off the type of the inner exception and this will break their code. That is the problem with throwing exceptions - there is no proper way to communicate which may possibly be thrown except documentation.

[michaelklishin]

Merging per discussion with @kjnilsson. We will decide on what version bump (if any) this should warrant at a later point.

[kjnilsson]

there is a pre release available that contain this change: https://www.nuget.org/packages/RabbitMQ.Client/5.1.0-pre2