[service-bus] Allow users to call open() on a Sender and initialize the connection
#8329
Conversation
Allows users to frontload the connection overhead and not have to see it on first send.
richardpark-msft
force-pushed
the
richardpark-sb-non-lazy-open
branch
from
d32ee74 to
346634f
Compare
| * Ensures that the Service Bus connection and sender link is open. | ||
| * | ||
| * This method will be called on-demand by the Sender. Only call this method if you need | ||
| * to force the opening of the connection and link. |
There was a problem hiding this comment.
This needs some work... I'd like us to convey that this is optional, not everyone should feel the need to use it. And a guidance on when one should consider calling it.
And also update our send code snippet in our readme and/or samples with something like the below:
Add
await sender.open()before your first send operation if you want to front load the work of setting up the AMQP link to the service. Doing this will save your first send operation from spending the time to do the same
There was a problem hiding this comment.
Agree with above, but it's AMQP connection and link I think?
There was a problem hiding this comment.
well, not always.
If you have used the client for any of the non send operations, then the AMQP connection is already up, all we need to do is set up the link
There was a problem hiding this comment.
In the interest of not going too much details, I would suggest to sticking to "link" only.
I would recommend to use my above suggestion in samples and the below for the docs on this public method
Opens the AMQP link to Azure Service Bus from the sender. It is not necessary to call this method in order to use the sender. It is recommended to call this before your first send() or sendBatch() calls if you want to front load the work of setting up the AMQP link to the service. Doing this will save your first send operation from spending the time to do the same.
There was a problem hiding this comment.
Updated, adding in some formatting.
I did remove the Doing this will save... part only because it seemed redundant with the prior sentences.
There was a problem hiding this comment.
(unresolving - didn't get to the sample yet)
There was a problem hiding this comment.
@ramya-rao-a - pre-critique (I don't want to push again and have my tests stop!)
(this goes into the ### Send messages snippet):
// Optionally, you can call sender.open() if you want to front load the work of setting
// up the AMQP link to the service. If not called, the sender will call open() on your behalf
// on the first send operation.
//
// await sender.open()
There was a problem hiding this comment.
"Optionally, you can call sender.open()" -> "Optionally, you can await on sender.open() "to double down that this is an async operation or "Optionally, you can call await sender.open()"
"on the first send operation." -> "in the first send operation." ?
|
What happens when user calls
|
I'm not sure if this is affected at all by what I've done, mostly because this code is the same as the code that was called before. So I suppose this would be very similar to what would happen if the user called (also, can't tell if you're saying - this is a new issue or an existing one)
This is totally legitimate - there's a lock that I need to migrate into the .open() call which will prevent this. In the next push. :) |
…id parallel open'ing.
|
@ramya-rao-a - I migrated the this.senderLink check into .open() which seems to do what we wanted. Let me know if that's not what you were talking about. |
- Wasn't properly awaiting in my lock test which was causing the tests to hang on exit - Change over to just purely using async rather than having a hybrid which could cause inconsistencies with the way errors came back. - Adding in a return type that I didn't mean to omit.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…o be removed as well.
- Fix some spelling mistakes in an unrelated doc comment
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…l async function _must_ call reject() explicitly. - Updating readme with some wording changes from Ramya.
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- Bring back missing log message for acquiring the lock - Update samples to show the .open() call
Adding an .open() method on Sender to allow users to control when connection and link initialization occur.
Fixes #3212