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.

Sign up for GitHub

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

[ServiceBus] Ensure link to non-existing subscription throws MessagingEntityNotFoundException #7942

Merged
merged 3 commits into from
Oct 12, 2019
Merged

[ServiceBus] Ensure link to non-existing subscription throws MessagingEntityNotFoundException #7942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a180dc3
Merge pull request #1 from Azure/master
nemakam Jul 30, 2019
abb859b
Merge pull request #2 from Azure/master
nemakam Oct 4, 2019
10f48fc
Ensure link to non-existing subscription throws MessagingEntityNotFou…
makam Oct 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/servicebus/Microsoft.Azure.ServiceBus/src/Amqp/AmqpLinkCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
exception);

session.SafeClose(exception);
throw AmqpExceptionHelper.GetClientException(exception, null, link?.GetInnerException(), session.IsClosing());
throw AmqpExceptionHelper.GetClientException(exception, null, link?.GetInnerException(), amqpConnection.IsClosing());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
14 changes: 14 additions & 0 deletions sdk/servicebus/Microsoft.Azure.ServiceBus/tests/ExpectedMessagingExceptionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,5 +194,19 @@ await ServiceBusScope.UsingQueueAsync(partitioned: false, sessionEnabled: true,
}
});
}

[Fact]
[LiveTest]
public async Task CreatingLinkToNonExistingEntityShouldThrowEntityNotFoundException()
{
var receiver = new MessageReceiver(TestUtility.NamespaceConnectionString, "nonExistingEntity"); // Covers queue and topic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@jsquire jsquire Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await Assert.ThrowsAsync<MessagingEntityNotFoundException>(async () => await receiver.ReceiveAsync());

await ServiceBusScope.UsingTopicAsync(partitioned: false, sessionEnabled: false, async (topicName, subscriptionName) =>
{
receiver = new MessageReceiver(TestUtility.NamespaceConnectionString, EntityNameHelper.FormatSubscriptionPath(topicName, "nonexistingsub"));
await Assert.ThrowsAsync<MessagingEntityNotFoundException>(async () => await receiver.ReceiveAsync());
});
}
}
}