[Event Hubs] Update send operation to include initialization

[ramya0820]

For more context refer to #2835

[ramya-rao-a]

@ramya0820 Am curious as to why we need 2 promises in the first place. Did you try having a single promise?

I would have imagined the older send function to include the below before attempting to send the event would have done the trick.

if (!this.isOpen()) {
    await defaultLock.acquire(this.senderLock, this._init).catch(reject);
}

cc @chradek @ShivangiReja

[ramya0820]

@ramya0820 Am curious as to why we need 2 promises in the first place. Did you try having a single promise?

Agree, so I wasn't aware that converting func to async one is a simple one (my bad!). I thought promise chaining was only way and tried to retain, re-use existing async-await code from within promise chaining.
Thanks to @chradek for pointing it out, the changes should now be simplified with having just one promise.

if (!this.isOpen()) {
await defaultLock.acquire(this.senderLock, this._init).catch(reject);
}

About this, the catch(reject) is unnecessary as the code is not within a callback, and the error throw would result in rejection of containing promise - correct? cc: @chradek

[chradek]

This looks good to me, can you confirm the tests are passing?

[chradek]

One change requested: call setTimeout before the init logic.

[ramya-rao-a]

Looking better! Left 1 comment. Please take a look at that and Chris's comments

[chradek]

Looks like build is failing because you removed the use of an imported method, but didn't remove the import.

[ramya-rao-a]

Please resolve the merge conflict, trigger a round of live tests and then merge the PR

[ramya-rao-a]

Due to the re-arrangement of the timer, if if (abortSignal && abortSignal.aborted) below returns true, then we end up returning a rejected promise (which is right) without clearing the timer.

Please consider keeping the previous order of the callbacks/helper-functions and only adding the link creation part at the right place

[ramya-rao-a]

The ordering in the recent commit of eaaf17e is better, but still needs work.

In the current setup, if the abort signal is fired when the async process of init() is in progress, then the promise is being rejected only after init() is complete. This is because init() is being called before the event handler for abort event was added.

Please make the below changes

• Add the event handler for abort before calling init() and before starting the timer
• Remove the event handler for abort before rejecting the promise when init() fails

[ramya0820]

#4322 (comment) thread specifically clarifies about abort during init().

This scenario if we want to address would then need to apply for the managementRequest as well correct? Or did we specifically exclude this because of complexity involved? (In not having access to removeListeners() on sender link from the SDK?) It just feels like UX is not consistent in both cases.

Okay to implement the optimization and new corner cases from this.

[ramya-rao-a]

This scenario applies to managementRequest as well i.e. the scenario of abort signal being fired when the async process of init() is in progress.

I have updated the comment thread for managementRequest. Please see https://github.com/Azure/azure-sdk-for-js/pull/4322/files#r305599979

[ramya-rao-a]

https://github.com/Azure/azure-sdk-for-js/pull/4319/files#r305173350

[ramya-rao-a]

/azp run js - eventhubs-client - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).