[core-rest-pipeline] Fix race condition in aborting request on Node (#…

…17956) Address an issue on Node where if we were still reading a response while an abort signal was triggered, the AbortError wouldn't be surfaced properly and instead a RestError would be raised.
Azure · Sep 30, 2021 · de8e547 · de8e547
1 parent ff2f53c
commit de8e547
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 15 deletions.
diff --git a/sdk/core/core-rest-pipeline/CHANGELOG.md b/sdk/core/core-rest-pipeline/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## 1.3.1 (2021-09-30)
 
+### Bugs Fixed
+
+- Addressed an issue on Node where aborting a request while its response body was still be processed would cause the HttpClient to emit a `RestError` rather than the appropriate `AbortError`. [PR #17956](https://github.com/Azure/azure-sdk-for-js/pull/17956)
+
 ### Other Changes
 
 - Updates package to work with the react native bundler. Browser APIs such as `URL` will still need to be pollyfilled for this package to run in react native. [PR #17783](https://github.com/Azure/azure-sdk-for-js/pull/17783)

diff --git a/sdk/core/core-rest-pipeline/src/nodeHttpClient.ts b/sdk/core/core-rest-pipeline/src/nodeHttpClient.ts
@@ -217,8 +217,9 @@ class NodeHttpClient implements HttpClient {
       });
 
       abortController.signal.addEventListener("abort", () => {
-        req.abort();
-        reject(new AbortError("The operation was aborted."));
+        const abortError = new AbortError("The operation was aborted.");
+        req.destroy(abortError);
+        reject(abortError);
       });
       if (body && isReadableStream(body)) {
         body.pipe(req);
@@ -229,7 +230,7 @@ class NodeHttpClient implements HttpClient {
           req.end(ArrayBuffer.isView(body) ? Buffer.from(body.buffer) : Buffer.from(body));
         } else {
           logger.error("Unrecognized body type", body);
-          throw new RestError("Unrecognized body type");
+          reject(new RestError("Unrecognized body type"));
         }
       } else {
         // streams don't like "undefined" being passed as data
@@ -313,11 +314,15 @@ function streamToText(stream: NodeJS.ReadableStream): Promise<string> {
       resolve(Buffer.concat(buffer).toString("utf8"));
     });
     stream.on("error", (e) => {
-      reject(
-        new RestError(`Error reading response as text: ${e.message}`, {
-          code: RestError.PARSE_ERROR
-        })
-      );
+      if (e && e?.name === "AbortError") {
+        reject(e);
+      } else {
+        reject(
+          new RestError(`Error reading response as text: ${e.message}`, {
+            code: RestError.PARSE_ERROR
+          })
+        );
+      }
     });
   });
 }

diff --git a/sdk/core/core-rest-pipeline/test/node/nodeHttpClient.spec.ts b/sdk/core/core-rest-pipeline/test/node/nodeHttpClient.spec.ts
@@ -15,12 +15,7 @@ class FakeResponse extends PassThrough {
   public headers?: IncomingHttpHeaders;
 }
 
-class FakeRequest extends PassThrough {
-  public finished?: boolean;
-  public abort(): void {
-    this.finished = true;
-  }
-}
+class FakeRequest extends PassThrough {}
 
 /**
  * Generic NodeJS streams accept typed arrays just fine,
@@ -53,7 +48,6 @@ function createResponse(statusCode: number, body = ""): IncomingMessage {
 
 function createRequest(): ClientRequest {
   const request = new FakeRequest();
-  request.finished = false;
   return (request as unknown) as ClientRequest;
 }
 
@@ -334,4 +328,40 @@ describe("NodeHttpClient", function() {
     const response = await promise;
     assert.strictEqual(response.status, 200);
   });
+
+  it("should return an AbortError when aborted while reading the HTTP response", async function() {
+    clock.restore();
+    const client = createDefaultHttpClient();
+    const controller = new AbortController();
+
+    const clientRequest = createRequest();
+    stubbedHttpsRequest.returns(clientRequest);
+    const request = createPipelineRequest({
+      url: "https://example.com",
+      abortSignal: controller.signal
+    });
+    const promise = client.sendRequest(request);
+
+    const streamResponse = new FakeResponse();
+
+    clientRequest.destroy = function(this: FakeRequest, e: Error) {
+      // give it some time to attach listeners and read from the stream
+      setTimeout(() => {
+        streamResponse.destroy(e);
+      }, 0);
+    };
+    streamResponse.headers = {};
+    streamResponse.statusCode = 200;
+    const buffer = Buffer.from("The start of an HTTP body");
+    streamResponse.write(buffer);
+    stubbedHttpsRequest.yield(streamResponse);
+    controller.abort();
+
+    try {
+      await promise;
+      assert.fail("Expected await to throw");
+    } catch (e) {
+      assert.strictEqual(e.name, "AbortError");
+    }
+  });
 });