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-lro] Set isCancelled when operation status is cancelled #21893

Merged
merged 12 commits into from
May 18, 2022
47 changes: 36 additions & 11 deletions sdk/core/core-lro/src/lroEngine/locationPolling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,20 @@ import {
LroResponse,
LroStatus,
RawResponse,
failureStates,
successStates,
} from "./models";
import { PollOperationState } from "../pollOperation";
import { isUnexpectedPollingResponse } from "./requestUtils";

function isPollingDone(rawResponse: RawResponse): boolean {
function getStatus(rawResponse: RawResponse): string {
const { status } = (rawResponse.body as LroBody) ?? {};
return typeof status === "string" ? status.toLowerCase() : "succeeded";
}

function isPollingDone(rawResponse: RawResponse, status: string): boolean {
if (isUnexpectedPollingResponse(rawResponse) || rawResponse.statusCode === 202) {
return false;
}
const { status } = (rawResponse.body as LroBody) ?? {};
const state = typeof status === "string" ? status.toLowerCase() : "succeeded";
if (isUnexpectedPollingResponse(rawResponse) || failureStates.includes(state)) {
throw new Error(`The long running operation has failed. The provisioning state: ${state}.`);
}
return successStates.includes(state);
return status === "succeeded";
}

/**
Expand All @@ -44,13 +43,39 @@ async function sendFinalRequest<TResult>(
}
}

export function processLocationPollingOperationResult<TResult>(
export function processLocationPollingOperationResult<
TResult,
TState extends PollOperationState<TResult>
>(
lro: LongRunningOperation<TResult>,
state: TState,
resourceLocation?: string,
lroResourceLocationConfig?: LroResourceLocationConfig
): (response: LroResponse<TResult>) => LroStatus<TResult> {
return (response: LroResponse<TResult>): LroStatus<TResult> => {
if (isPollingDone(response.rawResponse)) {
const rawResponse = response.rawResponse;
const status = getStatus(rawResponse);
if (isUnexpectedPollingResponse(rawResponse) || status === "failed") {
throw new Error(`The long running operation has failed.`);
}
/**
* These HTTP request methods are typically used around provisioned resources
* so if the LRO was cancelled, there is nothing useful can be returned to
* the customer. POST requests on the other hand could support partial results
* so throwing an error in this case could be prevent customer from getting
* access to those partial results.
*/
if (["PUT", "DELETE", "PATCH"].includes(lro.requestMethod) && status === "canceled") {
Copy link
Member

Choose a reason for hiding this comment

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

If we don't throw even in PUT, DELETE and PATCH. Would users get stuck in a weird state or just wouldn't have interesting information in the request?

If they don't end up in a bad place, would it make sense to not throw and have everything fall into the next check?

Copy link
Member Author

Choose a reason for hiding this comment

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

The pollUntilDone() method returns a result so if no error was thrown, it will return an object with a status field set to canceled as the result and it most likely will not match the expected type of the result object.

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 behavioral change for now and kept the original behavior of throwing in all cancellation scenarios. I will explore not throwing for POST requests in another PR.

throw new Error(`The long running operation has been canceled.`);
}
if (["canceled", "cancelled"].includes(status)) {
state.isCancelled = true;
return {
...response,
done: true,
};
}
if (isPollingDone(response.rawResponse, status)) {
if (resourceLocation === undefined) {
return { ...response, done: true };
} else {
Expand Down
3 changes: 2 additions & 1 deletion sdk/core/core-lro/src/lroEngine/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ export interface LroEngineOptions<TResult, TState> {
isDone?: (lastResponse: unknown, state: TState) => boolean;

/**
* A function to cancel the LRO.
* A function that takes the mutable state as input and attempts to cancel the
* LRO.
*/
cancel?: (state: TState) => Promise<void>;
}
Expand Down
15 changes: 12 additions & 3 deletions sdk/core/core-lro/src/lroEngine/operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,12 @@ export class GenericPollOperation<TResult, TState extends PollOperationState<TRe
...response,
done: isDone(response.flatResponse, this.state),
})
: createGetLroStatusFromResponse(this.lro, state.config, this.lroResourceLocationConfig);
: createGetLroStatusFromResponse(
this.lro,
state.config,
this.state,
this.lroResourceLocationConfig
);
this.poll = createPoll(this.lro);
}
if (!state.pollingURL) {
Expand Down Expand Up @@ -122,8 +127,12 @@ export class GenericPollOperation<TResult, TState extends PollOperationState<TRe
}

async cancel(): Promise<PollOperation<TState, TResult>> {
this.state.isCancelled = true;
await this.cancelOp?.(this.state);
const cancelOp =
this.cancelOp ??
(async (state: TState) => {
state.isCancelled = true;
});
await cancelOp(this.state);
return this;
}

Expand Down
5 changes: 4 additions & 1 deletion sdk/core/core-lro/src/lroEngine/stateMachine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,25 @@ import {
} from "./models";
import { getPollingUrl, inferLroMode, isUnexpectedInitialResponse } from "./requestUtils";
import { isBodyPollingDone, processBodyPollingOperationResult } from "./bodyPolling";
import { PollOperationState } from "../pollOperation";
import { logger } from "./logger";
import { processLocationPollingOperationResult } from "./locationPolling";
import { processPassthroughOperationResult } from "./passthrough";

/**
* creates a stepping function that maps an LRO state to another.
*/
export function createGetLroStatusFromResponse<TResult>(
export function createGetLroStatusFromResponse<TResult, TState extends PollOperationState<TResult>>(
lroPrimitives: LongRunningOperation<TResult>,
config: LroConfig,
state: TState,
lroResourceLocationConfig?: LroResourceLocationConfig
): GetLroStatusFromResponse<TResult> {
switch (config.mode) {
case "Location": {
return processLocationPollingOperationResult(
lroPrimitives,
state,
config.resourceLocation,
lroResourceLocationConfig
);
Expand Down
36 changes: 7 additions & 29 deletions sdk/core/core-lro/test/engine.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,7 @@ describe("Lro Engine", function () {
await runMockedLro("DELETE", `/delete${rootPrefix}/retry/canceled`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: canceled."
);
assert.equal(e.message, "The long running operation has been canceled.");
}
});

Expand All @@ -228,10 +225,7 @@ describe("Lro Engine", function () {
await runMockedLro("DELETE", `/delete${rootPrefix}/retry/failed`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: failed."
);
assert.equal(e.message, "The long running operation has failed.");
}
});

Expand All @@ -256,10 +250,7 @@ describe("Lro Engine", function () {
await runMockedLro("PUT", `/put${rootPrefix}/retry/failed`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: failed."
);
assert.equal(e.message, "The long running operation has failed.");
}
});

Expand Down Expand Up @@ -293,10 +284,7 @@ describe("Lro Engine", function () {
await runMockedLro("PUT", `/put${rootPrefix}/noretry/canceled`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: canceled."
);
assert.equal(e.message, "The long running operation has been canceled.");
}
});

Expand All @@ -321,10 +309,7 @@ describe("Lro Engine", function () {
await runMockedLro("POST", `/post${rootPrefix}/retry/failed`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: failed."
);
assert.equal(e.message, "The long running operation has failed.");
}
});

Expand All @@ -335,15 +320,8 @@ describe("Lro Engine", function () {
});

it("should handle postAsyncRetrycanceled", async () => {
try {
await runMockedLro("POST", `/post${rootPrefix}/retry/canceled`);
throw new Error("should have thrown instead");
} catch (e: any) {
assert.equal(
e.message,
"The long running operation has failed. The provisioning state: canceled."
);
}
const results = await runMockedLro("POST", `/post${rootPrefix}/retry/canceled`);
assert.equal(results.status, "Canceled");
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,6 @@ matrix([["APIKey", "AAD"]] as const, async (authMethod: AuthMethod) => {
assert.fail(`Operation has finished processing before requested to be cancelled`);
}
await originalPoller.cancelOperation();
assert.isTrue(originalPoller.getOperationState().isCancelled);
await assert.isRejected(originalPoller.pollUntilDone(), /Poller cancelled/);
});

Expand Down