[core-lro] Remove support for cancellation in LroEngine (Azure#22844)

deyaaeldeen · web-flow · commit 6eb5a1a76c46 · 2022-08-08T17:55:51.000Z
cancelOperation is being deprecated in Azure#22753 so `LroEngine` no longer needs to have support for it. Note that this support was introduced in 2.3.0-beta.1 so removing it is not a breaking change. Furthermore, this PR prepares for release today.
diff --git a/sdk/core/core-lro/CHANGELOG.md b/sdk/core/core-lro/CHANGELOG.md
@@ -1,17 +1,16 @@
 # Release History
 
-## 2.3.0-beta.2 (Unreleased)
-
-### Features Added
-
-### Breaking Changes
+## 2.2.5 (2022-08-08)
 
 ### Bugs Fixed
 
+- `LroEngine` no longer throws an error when it receives a 204 polling response.
+
 ### Other Changes
 
 - Support LROs with GET as the initial request method.
 - Better logging support for the operation and the poller.
+- Removed the unused dependency `@azure/core-tracing`.
 
 ## 2.3.0-beta.1 (2022-05-18)
 
diff --git a/sdk/core/core-lro/package.json b/sdk/core/core-lro/package.json
@@ -2,7 +2,7 @@
   "name": "@azure/core-lro",
   "author": "Microsoft Corporation",
   "sdk-type": "client",
-  "version": "2.3.0-beta.2",
+  "version": "2.2.5",
   "description": "Isomorphic client library for supporting long-running operations in node.js and browser.",
   "tags": [
     "isomorphic",
diff --git a/sdk/core/core-lro/review/core-lro.api.md b/sdk/core/core-lro/review/core-lro.api.md
@@ -25,7 +25,6 @@ export class LroEngine<TResult, TState extends PollOperationState<TResult>> exte
 
 // @public
 export interface LroEngineOptions<TResult, TState> {
-    cancel?: (state: TState) => Promise<void>;
     intervalInMs?: number;
     isDone?: (lastResponse: unknown, state: TState) => boolean;
     lroResourceLocationConfig?: LroResourceLocationConfig;
diff --git a/sdk/core/core-lro/src/lroEngine/lroEngine.ts b/sdk/core/core-lro/src/lroEngine/lroEngine.ts
@@ -42,8 +42,7 @@ export class LroEngine<TResult, TState extends PollOperationState<TResult>> exte
       options?.lroResourceLocationConfig,
       options?.processResult,
       options?.updateState,
-      options?.isDone,
-      options?.cancel
+      options?.isDone
     );
     super(operation);
 
diff --git a/sdk/core/core-lro/src/lroEngine/models.ts b/sdk/core/core-lro/src/lroEngine/models.ts
@@ -31,12 +31,6 @@ export interface LroEngineOptions<TResult, TState> {
    * A predicate to determine whether the LRO finished processing.
    */
   isDone?: (lastResponse: unknown, state: TState) => boolean;
-
-  /**
-   * A function that takes the mutable state as input and attempts to cancel the
-   * LRO.
-   */
-  cancel?: (state: TState) => Promise<void>;
 }
 
 /**
diff --git a/sdk/core/core-lro/src/lroEngine/operation.ts b/sdk/core/core-lro/src/lroEngine/operation.ts
@@ -40,8 +40,7 @@ export class GenericPollOperation<TResult, TState extends PollOperationState<TRe
     private lroResourceLocationConfig?: LroResourceLocationConfig,
     private processResult?: (result: unknown, state: TState) => TResult,
     private updateState?: (state: TState, lastResponse: RawResponse) => void,
-    private isDone?: (lastResponse: TResult, state: TState) => boolean,
-    private cancelOp?: (state: TState) => Promise<void>
+    private isDone?: (lastResponse: TResult, state: TState) => boolean
   ) {}
 
   public setPollerConfig(pollerConfig: PollerConfig): void {
@@ -135,7 +134,7 @@ export class GenericPollOperation<TResult, TState extends PollOperationState<TRe
   }
 
   async cancel(): Promise<PollOperation<TState, TResult>> {
-    await this.cancelOp?.(this.state);
+    logger.error("`cancelOperation` is deprecated because it wasn't implemented");
     return this;
   }
 
diff --git a/sdk/core/core-lro/src/poller.ts b/sdk/core/core-lro/src/poller.ts
@@ -309,34 +309,17 @@ export abstract class Poller<TState extends PollOperationState<TResult>, TResult
    * @param options - Optional properties passed to the operation's update method.
    */
   private async pollOnce(options: { abortSignal?: AbortSignalLike } = {}): Promise<void> {
-    try {
-      if (!this.isDone()) {
+    if (!this.isDone()) {
+      try {
         this.operation = await this.operation.update({
           abortSignal: options.abortSignal,
           fireProgress: this.fireProgress.bind(this),
         });
-        if (this.operation.state.isCancelled) {
-          this.stopped = true;
-          if (this.reject) {
-            this.reject(new PollerCancelledError("Poller cancelled"));
-          }
-          throw new Error(`The long-running operation has been canceled.`);
-        } else if (this.isDone() && this.resolve) {
-          // If the poller has finished polling, this means we now have a result.
-          // However, it can be the case that TResult is instantiated to void, so
-          // we are not expecting a result anyway. To assert that we might not
-          // have a result eventually after finishing polling, we cast the result
-          // to TResult.
-          this.resolve(this.operation.state.result as TResult);
-        }
+      } catch (e: any) {
+        this.operation.state.error = e;
       }
-    } catch (e: any) {
-      this.operation.state.error = e;
-      if (this.reject) {
-        this.reject(e);
-      }
-      throw e;
     }
+    this.processUpdatedState();
   }
 
   /**
@@ -379,13 +362,37 @@ export abstract class Poller<TState extends PollOperationState<TResult>, TResult
     return this.pollOncePromise;
   }
 
+  private processUpdatedState(): void {
+    if (this.operation.state.error) {
+      this.stopped = true;
+      this.reject!(this.operation.state.error);
+      throw this.operation.state.error;
+    }
+    if (this.operation.state.isCancelled) {
+      this.stopped = true;
+      const error = new PollerCancelledError("Poller cancelled");
+      this.reject!(error);
+      throw error;
+    } else if (this.isDone() && this.resolve) {
+      // If the poller has finished polling, this means we now have a result.
+      // However, it can be the case that TResult is instantiated to void, so
+      // we are not expecting a result anyway. To assert that we might not
+      // have a result eventually after finishing polling, we cast the result
+      // to TResult.
+      this.resolve(this.operation.state.result as TResult);
+    }
+  }
+
   /**
    * Returns a promise that will resolve once the underlying operation is completed.
    */
   public async pollUntilDone(): Promise<TResult> {
     if (this.stopped) {
       this.startPolling().catch(this.reject);
     }
+    // This is needed because the state could have been updated by
+    // `cancelOperation`, e.g. the operation is canceled or an error occurred.
+    this.processUpdatedState();
     return this.promise;
   }
 
diff --git a/sdk/core/core-lro/test/engine.spec.ts b/sdk/core/core-lro/test/engine.spec.ts
@@ -2098,127 +2098,4 @@ describe("Lro Engine", function () {
       assert.deepInclude(result, { id: "200", name: "foo" });
     });
   });
-
-  describe("poller cancellation", () => {
-    it("isCancel is set after the cancellation callback resolves", async () => {
-      const resourceLocationPath = "/LROPostDoubleHeadersFinalAzureHeaderGetDefault/location";
-      const pollingPath = "/LROPostDoubleHeadersFinalAzureHeaderGetDefault/asyncOperationUrl";
-      let run = false;
-      const poller = createPoller({
-        routes: [
-          {
-            method: "POST",
-            status: 202,
-            body: "",
-            headers: {
-              Location: resourceLocationPath,
-              "Operation-Location": pollingPath,
-            },
-          },
-          {
-            method: "GET",
-            path: pollingPath,
-            status: 200,
-            body: `{ "status": "running"}`,
-          },
-          {
-            method: "GET",
-            path: pollingPath,
-            status: 200,
-            body: `{ "status": "canceled" }`,
-          },
-        ],
-        cancel: async () => {
-          run = true;
-        },
-      });
-      assert.isUndefined(poller.getOperationState().isCancelled);
-      await poller.poll();
-      assert.isUndefined(poller.getOperationState().isCancelled);
-      await poller.cancelOperation();
-      assert.isTrue(run);
-      assert.isUndefined(poller.getOperationState().isCancelled);
-      await Promise.all([
-        assertError(poller.pollUntilDone(), {
-          messagePattern: /Poller cancelled/,
-        }),
-        assertError(poller.poll(), {
-          messagePattern: /The long-running operation has been canceled./,
-        }),
-      ]);
-      assert.isTrue(poller.getOperationState().isCancelled);
-    });
-
-    it("isCancel is not set when the cancellation callback throws", async () => {
-      const resourceLocationPath = "/LROPostDoubleHeadersFinalAzureHeaderGetDefault/location";
-      const pollingPath = "/LROPostDoubleHeadersFinalAzureHeaderGetDefault/asyncOperationUrl";
-      let run = false;
-      const poller = createPoller({
-        routes: [
-          {
-            method: "POST",
-            status: 202,
-            body: "",
-            headers: {
-              Location: resourceLocationPath,
-              "Operation-Location": pollingPath,
-            },
-          },
-          {
-            method: "GET",
-            path: pollingPath,
-            status: 200,
-            body: `{ "status": "succeeded"}`,
-          },
-          {
-            method: "GET",
-            path: resourceLocationPath,
-            status: 200,
-            body: `{ "id": "100", "name": "foo" }`,
-          },
-        ],
-        cancel: async () => {
-          run = true;
-          throw new Error();
-        },
-      });
-      assert.isUndefined(poller.getOperationState().isCancelled);
-      await poller.poll();
-      assert.isUndefined(poller.getOperationState().isCancelled);
-      await assert.isRejected(poller.cancelOperation());
-      assert.isTrue(run);
-      assert.isUndefined(poller.getOperationState().isCancelled);
-    });
-
-    it("cancelled poller gives access to partial results", async () => {
-      const pollingPath = "/LROPostDoubleHeadersFinalAzureHeaderGetDefault/asyncOperationUrl";
-      const poller = createPoller({
-        routes: [
-          {
-            method: "POST",
-            status: 202,
-            body: "",
-            headers: {
-              "Operation-Location": pollingPath,
-            },
-          },
-          {
-            method: "GET",
-            path: pollingPath,
-            status: 200,
-            body: `{ "status": "running"}`,
-          },
-          {
-            method: "GET",
-            path: pollingPath,
-            status: 200,
-            body: `{ "status": "canceled", "results": [1,2] }`,
-          },
-        ],
-      });
-      await assertError(poller.pollUntilDone(), { messagePattern: /Poller cancelled/ });
-      const result = poller.getResult();
-      assert.deepEqual(result!.results, [1, 2]);
-    });
-  });
 });
diff --git a/sdk/core/core-lro/test/utils/router.ts b/sdk/core/core-lro/test/utils/router.ts
@@ -116,9 +116,8 @@ export function createPoller<TState>(settings: {
   lroResourceLocationConfig?: LroResourceLocationConfig;
   processResult?: (result: unknown, state: TState) => Response;
   updateState?: (state: TState, lastResponse: RawResponse) => void;
-  cancel?: (state: TState) => Promise<void>;
 }): PollerLike<PollOperationState<Response>, Response> {
-  const { routes, lroResourceLocationConfig, processResult, updateState, cancel } = settings;
+  const { routes, lroResourceLocationConfig, processResult, updateState } = settings;
   const client = createClient(toLroProcessors(routes));
   const { method: requestMethod, path = initialPath } = routes[0];
   const lro = new CoreRestPipelineLro(createSendOp({ client }), {
@@ -134,7 +133,6 @@ export function createPoller<TState>(settings: {
     lroResourceLocationConfig,
     processResult,
     updateState,
-    cancel,
   });
 }
 
