[ServiceBus] Ensure link to non-existing subscription throws MessagingEntityNotFoundException #7942
Conversation
Merge from Azure/master
Merge from master
| [LiveTest] | ||
| public async Task CreatingLinkToNonExistingEntityShouldThrowEntityNotFoundException() | ||
| { | ||
| var receiver = new MessageReceiver(TestUtility.NamespaceConnectionString, "nonExistingEntity"); // Covers queue and topic |
There was a problem hiding this comment.
nit: This is a good candidate to break out into a unit test that is not bound to live Azure resources. Doing so will give you coverage during CI runs and PR validations; in the current form, this will only run with the nightly test passes.
There was a problem hiding this comment.
@jsquire, I'm not so sure how to convert this into a unit test without mocking the behavior of link.open().
In this case, the service is returning EntityNotFoundException on link.Open.
There was a problem hiding this comment.
You're absolutely correct.
This is my bad. I'm conflating the changes that were made to Event Hubs with those made for Service Bus. My response was to line 202 existing outside the scope... but the scenario is different than what was in my head. I had forgotten there is a static resident SB namespace present rather than it being created on the fly within the scope. Ignore this.
| @@ -86,7 +86,7 @@ protected AmqpLinkCreator(string entityPath, ServiceBusConnection serviceBusConn | |||
| exception); | |||
|
|
|||
| session.SafeClose(exception); | |||
| throw AmqpExceptionHelper.GetClientException(exception, null, link?.GetInnerException(), session.IsClosing()); | |||
| throw AmqpExceptionHelper.GetClientException(exception, null, link?.GetInnerException(), amqpConnection.IsClosing()); | |||
There was a problem hiding this comment.
It may be good to comment on this change; without digging into the context, it's not clear why we would use the connection over the session or vice-versa here.
There was a problem hiding this comment.
Essentially, the name of last arg is isConnectionError. We need to be able to distinguish between a regular error and a communication failure. We use connection.IsClosing flag to evaluate this value. When service returns an error, it generally returns only at link level (or at times, closes the entire link with the error). Closing of session most of the times is client side activity. For communication issues, connection is closed first, which eventually closed all the sessions and links. So using session.IsClosing does work for communication errors as well since if not for client, only communication exception generally leads to session closing.
But in this code, on link open exception, we also have a silly bug where we first trigger session.SafeClose() and then check if session.IsClosing(), which as Sean mentioned is always true.
There was a problem hiding this comment.
Thank you for the expanded context. This makes more sense to me now. It may be worth putting just a short mention in a code comment for those wondering.
There was a problem hiding this comment.
I did think of adding a comment, but can hardly come up with a comment which describes why I am using amqpConnection.IsClosing() for the argument isConnectionError.
The person who goes through the PR might have a confusion as to what's happening, but just looking at the current state of the code, it might not be a confusing thing.
jsquire
added
the
Client
Ensure opening link to non-existing subscription throws MessagingEntityNotFoundException
Fixes #6943