Add cancellation support to all async operations #66

ramya-rao-a · 2020-08-03T01:52:39Z

Description

Brief description of the changes made in the PR. This helps in making better changelog

This PR adds support for cancelling all the async operations via the use of the standard AbortSignalLike interface
This mainly involves clearing the associated timer at its base, followed by any other clean up that is needed.

Few design discussions:

AbortSignalLike interface is redefined in this PR instead of relying on an Azure scoped package like @azure/abort-controller which is used in the tests
I refrained from redefining the AbortError class due to potential conflicts with upstream usage. Instead, using same old Error class with custom name. This is definitely open for debate as to how we should approach this.

Reference to any github issues

Add support for abort signal for AwaitableSender.send() #48

ramya-rao-a · 2020-08-03T01:57:25Z

cc @richardpark-msft, @HarshaNalluru

richardpark-msft

This is an exciting change!

Just have some comments, assume we're waiting on a final review from @chradek.

lib/awaitableSender.ts

richardpark-msft · 2020-08-03T17:55:47Z

lib/awaitableSender.ts

@@ -187,12 +192,48 @@ export class AwaitableSender extends BaseSender {
 return reject(new OperationTimeoutError(message));
 }, this.sendTimeoutInSeconds * 1000);

+ const onAbort = () => {


Can we put this into a function rather than inlining it here?

Are you thinking of a function that accepts delivery as an argument, and then still in-lining, something like:

const onAbort = () => { this.abort(delivery); }

lib/connection.ts

richardpark-msft · 2020-08-03T18:04:03Z

lib/connection.ts

@@ -540,24 +628,27 @@ export class Connection extends Entity {
 *
 * @return Promise<AwaitableSender>.
 */
- async createAwaitableSender(options?: AwaitableSenderOptionsWithSession): Promise<AwaitableSender> {
+ async createAwaitableSender(options?: AwaitableSenderOptionsWithSession & { abortSignal?: AbortSignalLike; }): Promise<AwaitableSender> {


Why isn't abortSignal just in the AwaitableSenderOptionsWithSession?

AwaitableSenderOptionsWithSession extends AwaitableSenderOptions which extends BaseSenderOptions
SenderOptionsWithSession extends SenderOptions which extends BaseSenderOptions.
ReceiverOptionsWithSession extends ReceiverOptions

So, the right place to add the abortSignal would have been BaseSenderOptions and ReceiverOptions

But then, createRequestResponseLink() takes both SenderOptions and ReceiverOptions. Both options supporting abortSignal would be not be desirable for this method. So, I could either do the Pick/Omit magic, or keep abortSignal separate as done here. I chose the latter.

richardpark-msft · 2020-08-03T18:04:27Z

lib/connection.ts

 * @param {ReceiverOptionsWithSession} options Optional parameters to create a receiver link.
 * @return {Promise<Receiver>} Promise<Receiver>.
 */
- async createReceiver(options?: ReceiverOptionsWithSession): Promise<Receiver> {
+ async createReceiver(options?: ReceiverOptionsWithSession & { abortSignal?: AbortSignalLike; }): Promise<Receiver> {


Why isn't abortSignal just in the ReceiverOptionsWithSession?

Answered in https://github.com/amqp/rhea-promise/pull/66/files#r464826879

chradek

Thanks for adding cancellation support! My biggest question is if we're properly handling cancellation when the service responds with an error when we open/close.

It seems like if we remove the Error handler from whatever we're cancelling and the service does respond with an error (e.g. ConnectionError is response to ConnectionOpen or ConnectionClose), we could end up with an unhandled exception unless rhea allows the Error events to be fired without any listeners attached.

chradek · 2020-08-03T18:17:19Z

lib/awaitableSender.ts

@@ -187,12 +192,48 @@ export class AwaitableSender extends BaseSender {
 return reject(new OperationTimeoutError(message));
 }, this.sendTimeoutInSeconds * 1000);

+ const onAbort = () => {


Are you thinking of a function that accepts delivery as an argument, and then still in-lining, something like:

const onAbort = () => { this.abort(delivery); }

lib/awaitableSender.ts

lib/connection.ts

test/connection.spec.ts

lib/connection.ts

chradek · 2020-08-03T18:59:28Z

lib/connection.ts

@@ -366,6 +413,14 @@ export class Connection extends Entity {
 log.error("[%s] Connection got disconnected while closing itself: %O.", this.id, error);
 };

+ onAbort = () => {
+ removeListeners();


I'm curious here as well what happens if the server does close the connection with an error.

I believe the way rhea is set up is to fire events and not throw errors. If there are listeners, they respond, else the fired event gets ignored. There is no unhandled exceptions

chradek · 2020-08-04T16:27:16Z

I looked into why the CI is failing for this PR. It looks like the new tests don't set reconnect: false on the connections they create. When I ran the tests locally I found there were 2 timers hanging around related to rhea's built in reconnect logic. When I set reconnect to false for all the added tests those timers went away.

I didn't dig into exactly which tests were still allowing reconnects to occur, maybe there are some that don't explicitly close the connection so rhea attempts to reconnect.

ramya-rao-a · 2020-08-04T19:25:41Z

yay! Thanks for the hint @chradek!
Tests are green now

richardpark-msft · 2020-08-04T21:48:09Z

lib/connection.ts

 return new Promise((resolve, reject) => {
 if (!this.isOpen()) {

 let onOpen: Func<RheaEventContext, void>;
 let onClose: Func<RheaEventContext, void>;
+ let onAbort: Func<void, void>;
+ const abortSignal = options?.abortSignal;


I suppose this must work or else CI wouldn't run but the package.json says we're using TS 3.5.

That's because 3.5.1 is the minimum version 😄 But that does remind me that before a new version is published, we should evaluate if down-leveled types are needed.

Ah, right! :)

It's a dev dependency anyways - so should we bump it up?

Removed optional chaining in c12aeed to avoid rocking the boat regarding TS versions

chradek

My concerns were addressed, looks good to me!

Please make sure to look at #67 before publishing a new release.

richardpark-msft

Looks good. Someday we'll upgrade our TS version. :)

richardpark-msft · 2020-09-16T20:05:09Z

lib/awaitableSender.ts

 * @returns {Promise<Delivery>} Promise<Delivery> The delivery information about the sent message.
 */
- send(msg: Message | Buffer, tag?: Buffer | string, format?: number, timeoutInSeconds?: number): Promise<Delivery> {
+ send(msg: Message | Buffer, tag?: Buffer | string, format?: number, options?: AwaitableSendOptions): Promise<Delivery> {


This is not considered a breaking change?

timeoutInSeconds was introduced in #53 which has not been released yet. So, no breaking changes

richardpark-msft

Go go go!

HarshaNalluru

Looks good!

HarshaNalluru · 2020-09-16T22:31:09Z

Please add changelog

ramya-rao-a requested a review from chradek August 3, 2020 01:52

Add cancellation support to all async operations

d53dab8

ramya-rao-a force-pushed the abort-signal branch from 1216742 to d53dab8 Compare August 3, 2020 02:05

ramya-rao-a added 2 commits August 2, 2020 19:13

Use options where possible

6978967

Add support to cancel send()

a01480c

ramya-rao-a force-pushed the abort-signal branch from a39d6b7 to a01480c Compare August 3, 2020 02:41

richardpark-msft reviewed Aug 3, 2020

View reviewed changes

chradek reviewed Aug 3, 2020

View reviewed changes

ramya-rao-a added 6 commits August 3, 2020 15:15

createSession() takes options

2c9e833

onAbort need not be part of PromiseLike

111e48e

Use optional chaining on abortSignal

91920e7

Use helper to create error, fix logs

1198fcd

Fix comments as per feedback

2487cae

Remove the extra abort call

51c2055

Add reconnect: false to new connection in AbortSignal tests

4e077f2

richardpark-msft reviewed Aug 4, 2020

View reviewed changes

ramya-rao-a added 3 commits August 4, 2020 19:07

Throw AbortError when insufficient credits, but signal is already fired

a18c3b7

Use more of optional chaining

3d6248d

Check for entry in deliveryDispositionMap before rejecting promise

503399b

chradek approved these changes Aug 5, 2020

View reviewed changes

Avoid optional chainging as older TS versions dont support it

c12aeed

richardpark-msft approved these changes Aug 25, 2020

View reviewed changes

bterlson approved these changes Aug 31, 2020

View reviewed changes

ramya-rao-a added 2 commits August 31, 2020 11:27

Merge remote-tracking branch 'origin' into pr/ramya-rao-a/66

7e6c3db

Use AwaitbaleSendOptions

da1f64b

richardpark-msft reviewed Sep 16, 2020

View reviewed changes

richardpark-msft approved these changes Sep 16, 2020

View reviewed changes

HarshaNalluru approved these changes Sep 16, 2020

View reviewed changes

ramya-rao-a added 2 commits January 31, 2021 19:10

Use AbortSignalLike, not AbortSignal

48740af

Add changelog and update pkg version

5f0266e

ramya-rao-a force-pushed the abort-signal branch from 98b84fb to 5f0266e Compare February 1, 2021 03:14

ramya-rao-a mentioned this pull request Feb 1, 2021

Release new version #74

Closed

ramya-rao-a merged commit 68fdda1 into amqp:master Feb 1, 2021

ramya-rao-a deleted the abort-signal branch February 1, 2021 03:34

ramya-rao-a mentioned this pull request Jun 3, 2021

updates rhea-promise to 2.0.0 to match rhea dependency being updated to 2.x #87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cancellation support to all async operations #66

Add cancellation support to all async operations #66

ramya-rao-a commented Aug 3, 2020 •

edited

Loading

ramya-rao-a commented Aug 3, 2020

richardpark-msft left a comment

richardpark-msft Aug 3, 2020

chradek Aug 3, 2020

richardpark-msft Aug 3, 2020

ramya-rao-a Aug 4, 2020

richardpark-msft Aug 3, 2020

ramya-rao-a Aug 4, 2020

chradek left a comment

chradek Aug 3, 2020

chradek Aug 3, 2020

ramya-rao-a Aug 5, 2020

chradek commented Aug 4, 2020 •

edited

Loading

ramya-rao-a commented Aug 4, 2020

richardpark-msft Aug 4, 2020

chradek Aug 4, 2020

richardpark-msft Aug 4, 2020

ramya-rao-a Aug 25, 2020

chradek left a comment

richardpark-msft left a comment

richardpark-msft Sep 16, 2020

ramya-rao-a Sep 16, 2020

richardpark-msft left a comment

HarshaNalluru left a comment

HarshaNalluru commented Sep 16, 2020

Add cancellation support to all async operations #66

Add cancellation support to all async operations #66

Conversation

ramya-rao-a commented Aug 3, 2020 • edited Loading

Description

Reference to any github issues

ramya-rao-a commented Aug 3, 2020

richardpark-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Aug 4, 2020 • edited Loading

ramya-rao-a commented Aug 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek left a comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

HarshaNalluru left a comment

Choose a reason for hiding this comment

HarshaNalluru commented Sep 16, 2020

ramya-rao-a commented Aug 3, 2020 •

edited

Loading

chradek commented Aug 4, 2020 •

edited

Loading