AMQNET-848-Failover-Transport-Protocol-Excessive-Reconnection-Attempts

[muralim1969]

No description provided.

[Copilot]

Pull Request Overview

This PR adds handling for ExceptionResponse in InactivityMonitor to convert broker errors to NMSException (specifically triggering OnException on security exceptions), introduces a utility to map BrokerError objects to NMSException types, updates FailoverTransport to skip reconnects on security exceptions, and adds a new test for the security-exception path.

• Introduce ExceptionFromBrokerError to create NMSException from a broker error.
• Update InactivityMonitor.OnCommand to handle ExceptionResponse, calling OnException for NMSSecurityException and logging others.
• Modify FailoverTransport to avoid reconnect attempts on NMSSecurityException.
• Add OnCommand_WithNMSSecurityException_ShouldCallOnException test in InactivityMonitorTest.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
test/Transport/Inactivity/InactivityMonitorTest.cs	Added a testable subclass and test for security-exception handling in OnCommand.
src/Util/ExceptionFromBrokerError.cs	New utility to convert BrokerError into the correct NMSException.
src/Transport/InactivityMonitor.cs	Updated OnCommand to process ExceptionResponse and handle NMSSecurityException.
src/Transport/Failover/FailoverTransport.cs	Changed HandleTransportFailure to skip reconnects when encountering NMSSecurityException.

Comments suppressed due to low confidence (2)

src/Transport/InactivityMonitor.cs:236

• [nitpick] The variable name 'error' is ambiguous since it holds an ExceptionResponse. Consider renaming it to 'response' or 'exceptionResponse' for clarity.

                    ExceptionResponse error = command as ExceptionResponse;

src/Transport/InactivityMonitor.cs:244

• There is no test verifying the behavior when the broker exception is not an NMSSecurityException. Consider adding a test to assert that OnException is not called and a warning is logged for non-security exceptions.

                        Tracer.WarnFormat("ExceptionResponse received from the broker:", command.GetType());

[muralim1969]

@chirino Please have some one look at this PR

[Havret]

Hi @muralim1969,

Sorry, I’m not receiving notifications from this repo. I’ll try to take a look at your PR this week. I have some planned work (AMQNET-846) in this repo, so we could possibly cut a release that includes your changes as well.

Best,
Havret

[Havret]

thanks @muralim1969, lgtm

[Havret]

Hi @muralim1969, I had to revert your changes because they were causing the test TestRestartInvalidCredentialsWithFailover to fail. This test seems to cover the exact scenario you were trying to address. Here’s the PR: #45, which reverts the revert. This may need some additional work if you want to see it through.