[Event Hubs] Use AwaitableSender in lieu of Sender #4446
Conversation
|
I think you need to update the version of |
Discussed offline about this - the version update is part of #4228 and so changes/merge from master would fix the build errors (after which current PR can be merged). PR and code cannot be checked in anyway without build passing, hoping any concerns about the main issue, problem can be reviewed and surfaced as part of the review. Other than that, please see linked issue thread - #3375 (comment) |
| } | ||
|
|
||
| onAborted = () => { | ||
| removeListeners(); | ||
| rejectOnAbort(); |
There was a problem hiding this comment.
You'll want to add a return here or else the function will continue running.
There was a problem hiding this comment.
rejectOnAbort has a return reject(..) in it, would we still need another return here?
There was a problem hiding this comment.
Yes. rejectOnAbort is a function. The return in it only returns from that function and not the outer code set of {}
|
Hmm.. I see the discussion in #3375 (comment) and understand the trouble behind sharing the timeout value between link creation and actual sending of the message. This would need some changes in rhea-promise. For send operations by the user where the sender is already created, this will result in exactly what we want. For send operations by the user where the sender is not yet created, this will end up in doubling the net timeout. We can fix this once amqp/rhea-promise#49 is fixed. |
sdk/core/core-amqp/src/errors.ts
Outdated
| * Checks if given object maps to a valid custom error. If yes, configures and returns the appropriate error instance, else returns `undefined`. | ||
| * @param err | ||
| */ | ||
| function getCustomError(err: AmqpError | Error): MessagingError | undefined { |
There was a problem hiding this comment.
We don't want to cover any of the amqp errors here because amqp errors should undergo the appropriate processing as covered in the translate(). Therefore, make an early exit from here if isAmqpError(err) returns true
There was a problem hiding this comment.
I see that we have updated the if/else in translate() to use the result of this method only when it is not an amqp error. That is good.
But here since the input supports both AmqpError and simple Error, it is still confusing to anyone reading this code independently
My first suggestion would have been to update this method to take in only Error, but this might not work out due to typing issues. Because as far as the typescript compiler is concerned, the err object you pass in could be either of the 2 types.
Please consider not having this as a separate method and moving the code here directly into translate()
There was a problem hiding this comment.
Apart from #4446 (comment), the errors.ts file needs some work.
Other than looking good. Let's try to wrap this up tomorrow after a merge from the master branch
sdk/core/core-amqp/src/errors.ts
Outdated
| @@ -622,6 +639,8 @@ export function translate(err: AmqpError | Error): MessagingError { | |||
| error = new MessagingError("Websocket connection failed."); | |||
| error.name = ConditionErrorNameMapper[ErrorNameConditionMapper.ServiceCommunicationError]; | |||
| error.retryable = false; | |||
| } else if (customError) { | |||
| return customError; | |||
There was a problem hiding this comment.
Since we are creating the custom error up front and using it only after a series of if/elses, the current model slightly hurts code readability and the ease of understanding.
I would suggest the below
if (isAmqpError(err)) { // do the needful and return the error }
if (isSystemError(err)) { // do the needful and return the error }
if (isBrowserWebsocketError(err)) { // do the needful and return the error }
const customError = getCustomError(err)
if (customError) {
return customError
}
// The generic error handling hereThere was a problem hiding this comment.
Please take another look at the suggested code change above.
Apart from moving the call to getCustomError(), the suggestion was to add return the error from each of the if blocks and avoiding the else blocks.
With f5f0428, you have moved the call to getCustomError and removed the else but are not returning the error object from the previous if blocks.
Due to this regardless of all the processing that is done by if (isAmqpError(err)), if (isSystemError(err)) and if (isBrowserWebsocketError(err)) all the non custom errors are getting converted to a generic error. This is also the reason why the tests are failing
There was a problem hiding this comment.
The tests are failing. See https://github.com/Azure/azure-sdk-for-js/pull/4446/files#r309859510
Also, please respond to https://github.com/Azure/azure-sdk-for-js/pull/4446/files#r309415878
| newName: true | ||
| }); | ||
| const options: AwaitableSenderOptions = this._createSenderOptions( | ||
| Constants.defaultOperationTimeoutInMs |
There was a problem hiding this comment.
This is where we want to recreate the sender link with new name, therefore in the old code we used to pass newName: true to _createSenderOptions
In the updated code, we are no longer asking this. _createSenderOptions to use the new name
| @@ -682,13 +606,12 @@ export class EventHubSender extends LinkEntity { | |||
|
|
|||
| try { | |||
| await defaultLock.acquire(this.senderLock, () => { | |||
| return this._init(); | |||
| return this._init( | |||
| this._createSenderOptions(getRetryAttemptTimeoutInMs(options.retryOptions)) | |||
There was a problem hiding this comment.
The feedback at #4446 (comment) to avoid inlining applies here as well
…to issue-3375-p2
For more context refer to #3375