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

[Event Hubs] Use AwaitableSender in lieu of Sender #4446

Merged
merged 25 commits into from
Aug 2, 2019
Merged

[Event Hubs] Use AwaitableSender in lieu of Sender #4446

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
260b0e6
[Event Hubs] Introduce timeoutInMs on RetryOptions (#4239)
ramya0820 Jul 11, 2019
c74f5e4
Merge branch 'master' of https://github.com/ramya0820/azure-sdk-for-js
Jul 13, 2019
00016cb
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
ramya0820 Jul 15, 2019
5c12c13
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
ramya0820 Jul 16, 2019
2081256
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
Jul 17, 2019
f3511e0
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
ramya0820 Jul 19, 2019
7f00436
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
ramya0820 Jul 20, 2019
e2dc78b
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
Jul 23, 2019
232390c
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js
Jul 23, 2019
05046df
Replace Sender -> AwaitableSender
Jul 26, 2019
c58e6eb
Address comments
Jul 26, 2019
298990b
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
Jul 26, 2019
5956ffa
Remove comment
Jul 30, 2019
b2633be
Pass sendTimeout to AwaitableSender
Jul 30, 2019
264794c
Refactor translate()
Jul 30, 2019
aa664ff
Prepend rejects with return
Jul 31, 2019
25e3f63
Refactor translate()
Jul 31, 2019
edf1949
Add tests
Jul 31, 2019
ebd7dde
Refactor and fix timeout to init()
Jul 31, 2019
11dbe10
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
Jul 31, 2019
cb14ba3
Address comments
Aug 1, 2019
f5f0428
Address comments
Aug 1, 2019
abc9689
Refactor translate()
Aug 2, 2019
ffca3c9
Address comments
Aug 2, 2019
dee862c
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…
Aug 2, 2019
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
61 changes: 40 additions & 21 deletions sdk/core/core-amqp/src/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,12 +481,20 @@ export const retryableErrors: string[] = [
"ServerBusyError",
"ServiceUnavailableError",
"OperationCancelledError",

// OperationTimeoutError occurs when the service fails to respond within a given timeframe.
// Since reasons for such failures can be transient, this is treated as a retryable error.
"OperationTimeoutError",

"SenderBusyError",
"MessagingError",
"DetachForcedError",
"ConnectionForcedError",
"TransferLimitExceededError"
"TransferLimitExceededError",

// InsufficientCreditError occurs when the number of credits available on Rhea link is insufficient.
// Since reasons for such shortage can be transient such as for pending delivery of messages, this is treated as a retryable error.
"InsufficientCreditError"
];

/**
Expand Down Expand Up @@ -539,6 +547,32 @@ function isBrowserWebsocketError(err: any): boolean {
return result;
}

/**
* @internal
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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()

const error: MessagingError = err as MessagingError;
const errorName = (err as Error).name;
if (
// instanceof checks on custom Errors doesn't work without manually setting the prototype within the error.
// Must do a name check until the custom error is updated, and that doesn't break compatibility
// https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
retryableErrors.indexOf(errorName) > -1
) {
error.retryable = true;
return error;
}

if (errorName === "AbortError") {
error.retryable = false;
return error;
}

return undefined;
}

/**
* Translates the AQMP error received at the protocol layer or a generic Error into a MessagingError.
*
Expand All @@ -553,28 +587,9 @@ export function translate(err: AmqpError | Error): MessagingError {

let error: MessagingError = err as MessagingError;

// OperationTimeoutError occurs when the service fails to respond within a given timeframe.
// Since reasons for such failures can be transient, this is treated as a retryable error.
if (
// instanceof checks on custom Errors doesn't work without manually setting the prototype within the error.
// Must do a name check until OperationTimeoutError is updated, and that doesn't break compatibility
// https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
(err as Error).name === "OperationTimeoutError"
) {
error.retryable = true;
return error;
}

// Built-in errors like TypeError and RangeError should not be retryable as these indicate issues
// with user input and not an issue with the Messaging process.
if (
err instanceof TypeError ||
err instanceof RangeError ||
// instanceof checks on custom Errors doesn't work without manually setting the prototype within the error.
// Must do a name check until AbortError is updated, and that doesn't break compatibility
// https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
(err as Error).name === "AbortError"
) {
if (err instanceof TypeError || err instanceof RangeError) {
error.retryable = false;
return error;
}
Expand Down Expand Up @@ -622,6 +637,10 @@ export function translate(err: AmqpError | Error): MessagingError {
error = new MessagingError("Websocket connection failed.");
error.name = ConditionErrorNameMapper[ErrorNameConditionMapper.ServiceCommunicationError];
error.retryable = false;
}
const customError = getCustomError(err);
if (customError) {
return customError;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 here

Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else {
// Translate a generic error into MessagingError.
error = new MessagingError((err as Error).message);
Expand Down
46 changes: 34 additions & 12 deletions sdk/core/core-amqp/test/errors.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as chai from "chai";
const should = chai.should();

import * as Errors from "../src/errors";
import { AbortError } from "@azure/abort-controller";

class AMQPError {
name = "AmqpProtocolError";
Expand All @@ -30,6 +31,35 @@ describe("Errors", function() {
translatedError.message.should.equal(ehError.message);
});

it("Sets retryable to true, if input is custom error and name is OperationTimeoutError", function() {
const err = new Error("error message");
err.name = "OperationTimeoutError";
const translatedError = Errors.translate(err);
should.equal(translatedError.name === "OperationTimeoutError", true);
translatedError.message.should.equal(err.message);
translatedError.stack!.should.equal(err.stack);
translatedError.retryable.should.equal(true);
});

it("Sets retryable to true, if input is custom error and name is InsufficientCreditError", function() {
const err = new Error("error message");
err.name = "InsufficientCreditError";
const translatedError = Errors.translate(err);
should.equal(translatedError.name === "InsufficientCreditError", true);
translatedError.message.should.equal(err.message);
translatedError.stack!.should.equal(err.stack);
translatedError.retryable.should.equal(true);
});

it("Sets retryable to false, if input is the custom AbortError", function() {
const err = new AbortError("error message");
const translatedError = Errors.translate(err);
should.equal(translatedError.name === "AbortError", true);
translatedError.message.should.equal(err.message);
translatedError.stack!.should.equal(err.stack);
translatedError.retryable.should.equal(false);
});

it("Sets retryable to false, and acts as a passthrough if the input is TypeError", function() {
const err = new TypeError("This is a wrong type!!");
const translatedError = Errors.translate(err);
Expand Down Expand Up @@ -67,10 +97,7 @@ describe("Errors", function() {
{ from: "<unknown>", to: "MessagingError" }
].forEach(function(mapping) {
it("translates " + mapping.from + " into " + mapping.to, function() {
const err: any = new AMQPError(
mapping.from as any,
mapping.message as any
);
const err: any = new AMQPError(mapping.from as any, mapping.message as any);
const translatedError = <Errors.MessagingError>Errors.translate(err);
translatedError.name.should.equal(mapping.to);
if (
Expand Down Expand Up @@ -113,18 +140,13 @@ describe("Errors", function() {
code: "ESOMETHINGRANDOM",
errno: "ESOMETHINGRANDOM",
syscall: "read",
message:
"code: ESOMETHINGRANDOM, errno: ESOMETHINGRANDOM, syscall: read"
message: "code: ESOMETHINGRANDOM, errno: ESOMETHINGRANDOM, syscall: read"
}
].forEach(function(mapping) {
it(
"SystemError from node.js with code: '" +
mapping.code +
"' to a MessagingError",
"SystemError from node.js with code: '" + mapping.code + "' to a MessagingError",
function() {
const translatedError = <Errors.MessagingError>(
Errors.translate(mapping as any)
);
const translatedError = <Errors.MessagingError>Errors.translate(mapping as any);
if (mapping.code === "ECONNRESET") {
translatedError.name.should.equal("ServiceUnavailableError");
translatedError.retryable.should.equal(true);
Expand Down
2 changes: 1 addition & 1 deletion sdk/eventhub/event-hubs/review/event-hubs.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { AbortSignalLike } from '@azure/abort-controller';
import { AmqpError } from 'rhea-promise';
import { AwaitableSender } from 'rhea-promise';
import { ConnectionContextBase } from '@azure/core-amqp';
import { DataTransformer } from '@azure/core-amqp';
import { DefaultDataTransformer } from '@azure/core-amqp';
Expand All @@ -16,7 +17,6 @@ import { MessagingError } from '@azure/core-amqp';
import { Receiver } from 'rhea-promise';
import { ReceiverOptions } from 'rhea-promise';
import { RetryPolicy } from '@azure/core-amqp';
import { Sender } from 'rhea-promise';
import { SharedKeyCredential } from '@azure/core-amqp';
import { TokenCredential } from '@azure/core-amqp';
import { TokenType } from '@azure/core-amqp';
Expand Down
Loading