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.

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

[core-util] Abstract the abortable promise pattern #24821

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
12 changes: 7 additions & 5 deletions common/config/rush/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions sdk/core/core-util/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,13 @@
"@azure/dev-tool": "^1.0.0",
"@microsoft/api-extractor": "^7.31.1",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.4",
"@types/mocha": "^7.0.2",
"@types/node": "^14.0.0",
"@types/sinon": "^9.0.4",
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"downlevel-dts": "^0.10.0",
"cross-env": "^7.0.2",
"eslint": "^8.0.0",
Expand Down
80 changes: 59 additions & 21 deletions sdk/core/core-util/src/delay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,71 @@ export interface DelayOptions {
abortErrorMsg?: string;
}

/**
*
* @param inputs - The inputs for creating an abortable promise are the
* buildPromise function and the cleanupBeforeAbort function.
* buildPromise takes both the resolve and reject functions as
* parameters. cleanupBeforeAbort is called right before the
* promise is rejected.
* @returns a function that takes an optional DelayOptions parameter and returns
* a promise that can be aborted.
* @internal
*/
export function createAbortablePromise<T>(inputs: {
buildPromise: (inputs: {
Copy link
Member

Choose a reason for hiding this comment

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

nit: typically we'd have the required argument be first and optional arguments be on a separate options object. I don't think it should be mandatory, but I'm curious why we took an object-first approach here

Copy link
Member Author

@deyaaeldeen deyaaeldeen Feb 11, 2023

Choose a reason for hiding this comment

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

tbh I like the object-first approach because it forces me to name everything so the client code becomes more readable but I agree with you we should stick with our convention if we're going to export it eventually. Addressed in 64ea6c2.

resolve: (value: T | PromiseLike<T>) => void;
Copy link
Member

Choose a reason for hiding this comment

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

what about typing the buildPromise more like the original Promise constructor? something like:

buildPromise: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void;

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, addressed in 64ea6c2

reject: (reason?: any) => void;
}) => void;
cleanupBeforeAbort?: () => void;
}): (options?: DelayOptions) => Promise<T> {
const { buildPromise, cleanupBeforeAbort } = inputs;
Copy link
Member

@xirzec xirzec Feb 10, 2023

Choose a reason for hiding this comment

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

if you like you can destructure in the method declaration to avoid having to have the awkward name inputs:

export function createAbortablePromise<T> ({ buildPromise, cleanupBeforeAbort }: OptionType): ReturnType {

return ({ abortSignal, abortErrorMsg } = {}) =>
Copy link
Member

Choose a reason for hiding this comment

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

what about having the abortSignal and the abortErrorMsg be part of the original options bag instead of returning a function here? I feel like it's a little nicer to be able to implement delay like this:

  return createAbortablePromise<void>({
    buildPromise: ({ resolve }) => {
      token = setTimeout(resolve, timeInMs);
    },
    cleanupBeforeAbort: () => clearTimeout(token),
    abortSignal,
    abortErrorMsg: abortErrorMsg ?? StandardAbortMessage,
  });

or if we use my other suggestions:

  return createAbortablePromise<void>(
    (resolve) => {
      token = setTimeout(resolve, timeInMs);
    }, {
      onAbortCalled: () => clearTimeout(token),
      abortSignal,
      abortErrorMsg
    }
  );

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, the customer can create the function themselves if they need to. Addressed in 64ea6c2

new Promise((resolve, reject) => {
function rejectOnAbort(): void {
reject(new AbortError(abortErrorMsg ?? "The operation was aborted."));
Copy link
Member

Choose a reason for hiding this comment

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

re-use the StandardAbortMessage constant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated that constant earlier to be unique to the delay function, so now it reads "The delay was aborted". It wouldn't work for this general-purpose function.

}
function removeListeners(): void {
abortSignal?.removeEventListener("abort", onAbort);
}
function onAbort(): void {
cleanupBeforeAbort?.();
Copy link
Member

Choose a reason for hiding this comment

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

not sure if it's a valid scenario: do we ever need to await some async cleanup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Typically you don't want to block on cleanup and it is an anti-pattern to await inside the promise executer because error handling becomes harder. @xirzec do you have thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Yes, I've been bitten by blocked calls in Service Bus closing code...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't like "event" style callbacks to be awaited, it creates hard to reason about systems. We could always rename this to be more obvious like onAbortCalled or something

removeListeners();
rejectOnAbort();
}
if (abortSignal?.aborted) {
return rejectOnAbort();
}
buildPromise({
Copy link
Member

Choose a reason for hiding this comment

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

One subtlety here is the promise constructor catches exceptions in the callback, so a promise created like this:

const promise = new Promise(() => { throw new Error("oh no"));

will correctly return a rejected promise with the captured error instead of the constructor itself throwing.

To keep this contract consistent, can we put a try/catch around the call to buildPromise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we shouldn't assume that reject will be called appropriately inside.

resolve: (x) => {
removeListeners();
Copy link
Member

Choose a reason for hiding this comment

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

does the order matter? the old delay code removes listeners before resolving.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted the ordering but I don't think it matters.

resolve(x);
},
reject: (x) => {
removeListeners();
reject(x);
},
});
abortSignal?.addEventListener("abort", onAbort);
});
}

/**
* A wrapper for setTimeout that resolves a promise after timeInMs milliseconds.
* @param timeInMs - The number of milliseconds to be delayed.
* @param options - The options for delay - currently abort options
* @returns Promise that is resolved after timeInMs
*/
export function delay(timeInMs: number, options?: DelayOptions): Promise<void> {
return new Promise((resolve, reject) => {
function rejectOnAbort(): void {
reject(new AbortError(options?.abortErrorMsg ?? StandardAbortMessage));
}
function removeListeners(): void {
options?.abortSignal?.removeEventListener("abort", onAbort);
}
function onAbort(): void {
// eslint-disable-next-line @typescript-eslint/no-use-before-define
clearTimeout(token);
removeListeners();
rejectOnAbort();
}
if (options?.abortSignal?.aborted) {
return rejectOnAbort();
}
const token = setTimeout(() => {
removeListeners();
resolve();
}, timeInMs);
options?.abortSignal?.addEventListener("abort", onAbort);
let token: ReturnType<typeof setTimeout>;
const { abortSignal, abortErrorMsg } = options || {};
return createAbortablePromise<void>({
buildPromise: ({ resolve }) => {
token = setTimeout(resolve, timeInMs);
},
cleanupBeforeAbort: () => clearTimeout(token),
})({
abortSignal,
abortErrorMsg: abortErrorMsg ?? StandardAbortMessage,
});
}
45 changes: 45 additions & 0 deletions sdk/core/core-util/test/internal/createAbortablePromise.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import * as sinon from "sinon";
import { AbortController } from "@azure/abort-controller";
import chai from "chai";
import chaiAsPromised from "chai-as-promised";
import { createAbortablePromise } from "../../src/delay";

chai.use(chaiAsPromised);
const { assert } = chai;

describe("createAbortablePromise", function () {
let token: ReturnType<typeof setTimeout>;
const delayTime = 2500;
const createPromise = createAbortablePromise<void>({
buildPromise: ({ resolve }) => {
token = setTimeout(resolve, delayTime);
},
cleanupBeforeAbort: () => clearTimeout(token),
});
afterEach(function () {
sinon.restore();
});

it("should resolve if not aborted nor rejected", async function () {
const clock = sinon.useFakeTimers();
const promise = createPromise();
const time = await clock.nextAsync();
clock.restore();
assert.strictEqual(time, delayTime);
await assert.isFulfilled(promise);
});

it("should reject when aborted", async function () {
const aborter = new AbortController();
const abortErrorMsg = "The operation was aborted.";
Copy link
Member

Choose a reason for hiding this comment

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

could make this a unique message to test the message actually gets used instead of the standard one?

const promise = createPromise({
abortSignal: aborter.signal,
abortErrorMsg,
});
aborter.abort();
await assert.isRejected(promise, new RegExp(abortErrorMsg));
Copy link
Member

Choose a reason for hiding this comment

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

I think the new RegExp isn't necessary here as isRejected can take a string directly

});
});