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

[Service Bus] Fix MaxListenersExceededWarning by avoiding adding too many listeners #11175

Merged
merged 1 commit into from
Sep 10, 2020
Merged

[Service Bus] Fix MaxListenersExceededWarning by avoiding adding too many listeners #11175

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion sdk/servicebus/service-bus/src/core/managementClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,9 @@ export class ManagementClient extends LinkEntity<RequestResponseLink> {
);

try {
await this._init(sendRequestOptions?.abortSignal);
if (!this.isOpen()) {
await this._init(sendRequestOptions?.abortSignal);
}
Copy link
Member

Choose a reason for hiding this comment

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

This fix should work.

I think the issue here is that the two event listeners that are being added should have been passed in as part of the respective options for their links, much like this one is for session error:

azure-sdk-for-js/sdk/servicebus/service-bus/src/core/managementClient.ts

Line 232 in 8fb7f4e

onSessionError: (context: EventContext) => {

(and yes, I totally missed this during my refactor. We can add in some testing for this and possibly a longer term fix as a later PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

@richardpark-msft, Can you log an issue for the said longer term fix or do we want to re-use the existing one?

Copy link
Member Author

@HarshaNalluru HarshaNalluru Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the issue here is that the two event listeners that are being added should have been passed in as part of the respective options for their links, much like this one is for session error

Agree with that. I did think in those lines, though didn't spend much time on how. Moreover, initializing the link for each request shouldn't happen even if we tie the listeners and since that fixes the current issue, went ahead just with it.

} finally {
clearTimeout(waitTimer);
}
Expand Down