[Service Bus] Fix MaxListenersExceeded for management client #11738
Conversation
HarshaNalluru
requested review from
chradek,
ramya-rao-a and
richardpark-msft
as code owners
ghost
added
the
| ReceiverEvents.receiverError | ||
| ); | ||
| if (senderErrorListenerCount && senderErrorListenerCount < 1) { | ||
| this.link!.sender.on(SenderEvents.senderError, (context: EventContext) => { |
There was a problem hiding this comment.
There are options that you can pass to initLink for callbacks that are intended to be only set once - if the ones you're setting aren't there we should add them and register them there.
That should eliminate the need to do this kind of checking.
There was a problem hiding this comment.
I did look into that. There are complications, however.
"rhea" doesn't allow setting only the "onError" handler in the options if it is not accompanied by an "onMessage" handler.
Our onMessage handler("message" event) is set at core-amqp and we are trying to pass the onError handler in the options.
If we want to pass both onError and onMessage through core-amqp, we'd need to update the core-amqp's logic drastically, which would also impact event-hubs I guess.
There was a problem hiding this comment.
You have the lock to protect you here so you can do one-time init-only modification to the receiver.
There was a problem hiding this comment.
Good idea, updated createRheaLink instead to add the listener and thus, this would happen as a one-time init-only modification.
…to harshan/sb/issue/renewlock-listener-exceeded-warning
…king for the count before adding the listener
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…to harshan/sb/issue/renewlock-listener-exceeded-warning
| @@ -13,9 +13,12 @@ | |||
|
|
|||
| - `NamespaceProperties` interface property "messageSku" type changed from "string" to string literal type "Basic" | "Premium" | "Standard". [PR 11810](https://github.com/Azure/azure-sdk-for-js/pull/11810) | |||
|
|
|||
| - Internal improvement - For the operations depending on `$management` link such as peek or lock renewals, the listeners for the "sender_error" and "receiver_error" events were added to the link for each new request made before the link is initialized. This has been improved such that the listeners are reused. | |||
There was a problem hiding this comment.
Might be worth mentioning the error message that would have popped up (for people searching).
There was a problem hiding this comment.
Minor changelog suggestion but otherwise looks good.
I'm interested if you have a way of testing this - I believe we did, on occasion, see this warning in our tests. Perhaps it's something we can pick up in our stress tests (ie, settle many messages outside of the receiver link).
Sounds good as a stress scenario, I'll add to the list, however I don't think there are any good ways to test other than |
Portal revision API (Azure#11738) * Portal revision API * fixed spec errors * fixed references * fixed return code on create and added long running operation properties * fixed create response * fixed comments * small fix
HarshaNalluru
deleted the
harshan/sb/issue/renewlock-listener-exceeded-warning
branch
Issue
#11736
Cause
Consider renewlock attempts being made in parallel for multiple messages, and the management link is not initialized.
_initis called from the_makeManagementRequestmethod multiple times since the AMQP link is not open._initcalls.MaxListenersExceededgets thrown.Fix
Updating the "adding listeners" code inside
_initundermanagementClientwith the if-checks to add the listeners only if they are not present already.There are other ways to solve the issue, but this is probably the simplest and least disruptive.