search

Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.07k stars 1.19k forks source link

[Service Bus] Looks like the sender doesn't close when the client is closed #24529

Open HarshaNalluru opened 1 year ago

HarshaNalluru commented 1 year ago

Found while investigating the perf tests at https://github.com/Azure/azure-sdk-for-js/pull/23819

What did the test do & How to repro

Global setup part of the test contains creation of a sender from a SB Client and messages are sent. At the end of the test, client is closed, which meant the underlying sender is also closed.

However, the test hang in the 2nd iteration(2nd clean run) of the pipeline.

When the sender.close() step was added, the test started playing fine. Fix in the test - https://github.com/Azure/azure-sdk-for-js/pull/23819/commits/ec681c3419c20c1ecd52ee4fd5652ec6f6769f01

What to investigate

When the client is closed, the underlying sender should also get closed. Investigate why it didn't happen and make sure to fix the bug.

jeremymeng commented 1 year ago

interesting! Looks that we close the underlying MessageSenders that are tracked in the connection context,

https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/servicebus/service-bus/src/connectionContext.ts#L630

but calling ServiceBusSender.close directly set _isClosed to true in addtion.

https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/servicebus/service-bus/src/sender.ts#L381

at a quick glance, I didn't see how that would matter though.

HarshaNalluru commented 1 year ago

Yeah, I am clueless too, I checked twice. I was damn sure the sender must be closed, but somehow adding that extra line made our perf pipelines better.

Worst case, it could just be a flaky pipeline issue, unsure. I just didn't want to take the chance to rule this issue out of the possibilities yet.