[core-http] use response status to determine whether body is stream #13192

jeremymeng · 2021-01-13T01:17:19Z

Our current code has the incorrect assumption that if any body mapper
of an operation spec is of Stream type then response body must be a
stream and httpRequest.streamResponseBody is set according to this
assumption. However, Storage issue #12997 shows that the default
response of Download Blob operation does not have a stream body with
the following operation spec:

  responses: {
    200: {
      bodyMapper: {
        serializedName: "parsedResponse",
        type: {
          name: "Stream"
        }
      },
      headersMapper: Mappers.BlobDownloadHeaders
    },
    ...
    default: {
      bodyMapper: Mappers.StorageError,
      headersMapper: Mappers.BlobDownloadHeaders
    }
  },

Treating response body as stream in the default case prevent us from
parsing the body which leads to missing code and message from the
error object.

This change fixes the issue by actually checking the body type of the
response corresponding to the response status code.

WebResource{Like}.streamResponseBody is deprecated
because a boolean value is not enough to determine whether a response
is streaming or not. A list of streaming response status code
streamResponseStatusCodes is introduced and used instead.

ServiceClient no longer sets the deprecated
streamResponsebody property based on incorrect assumption. However
we still respect streamResponseBody in case it's set by users.

Core v2 removes streamResponseBody.

Our current code has the incorrect assumption that if any body mapper of an operation spec is of `Stream` type then response body must be a stream and `httpRequest.streamResponseBody` is set according to this assumption. However, Storage issue Azure#12997 shows that the default response of Download Blob operation does not have a stream body with the following operation spec: ```typescript responses: { 200: { bodyMapper: { serializedName: "parsedResponse", type: { name: "Stream" } }, headersMapper: Mappers.BlobDownloadHeaders }, ... default: { bodyMapper: Mappers.StorageError, headersMapper: Mappers.BlobDownloadHeaders } }, ``` Treating response body as stream in the default case prevent us from parsing the body which leads to missing `code` and `message` from the error object. This change fixes the issue by actually checking the body type of the response corresponding to the response status code.

jeremymeng · 2021-01-13T01:19:27Z

sdk/core/core-http/src/fetchHttpClient.ts

      const operationResponse: HttpOperationResponse = {
        headers: headers,
        request: httpRequest,
        status: response.status,
-        readableStreamBody: httpRequest.streamResponseBody


We probably also want to remove all usage of httpRequest.streamResponseBody? It's in public surface but I don't see other usages than checking for response body type and doing it incorrectly.

Certainly we should remove it from core-https at least.

Looking at it again. It is used in browser xhrHttpClient to have different code paths to resolve the responses so I will keep it.

xirzec

Change seems good, but can you please port it over to core-https as well? 😄

xirzec · 2021-01-13T18:26:34Z

sdk/core/core-http/src/fetchHttpClient.ts

      const operationResponse: HttpOperationResponse = {
        headers: headers,
        request: httpRequest,
        status: response.status,
-        readableStreamBody: httpRequest.streamResponseBody


Certainly we should remove it from core-https at least.

xirzec · 2021-01-13T18:27:24Z

sdk/core/core-http/src/fetchHttpClient.ts

@@ -153,14 +154,20 @@ export abstract class FetchHttpClient implements HttpClient {
      const response: CommonResponse = await this.fetch(httpRequest.url, requestInit);

      const headers = parseHeaders(response.headers);
+
+      const isBodyReallyStream = isResponseBodyStream(


Can we come up with a better variable name here? Perhaps bodyIsStream ?

jeremymeng · 2021-01-14T19:52:05Z

sdk/core/core-http/src/fetchHttpClient.ts

@@ -153,14 +154,20 @@ export abstract class FetchHttpClient implements HttpClient {
      const response: CommonResponse = await this.fetch(httpRequest.url, requestInit);

      const headers = parseHeaders(response.headers);
+


@xirzec in core v2 this is in core-https. I don't think it deals with operation specs so we wouldn't be able to check the response type based on status code. Any suggestions?

EDITED: typo

Oh. So is the only way to detect the response shape based on what we think we know about the response from the operation spec? Seems like there should be a way to detect this without prior knowledge.

It is the current way. Based on the types from the operation spec, we put the body into either readableStreamBody/blobBody, or bodyAsText property. I can think of some heuristics, for example,

treat as text if content type header value is one of the several that we would parse, or is plain text. otherwise treat as stream. For text cases we would store response.text() of the original response in bodyAsText so hopefully no matter how the body is transfer back we would have text at the end.

Anyway it feels like core v2 needs a different fix than porting this PR. I will log a separate issue to track it.

logged #13235

Change log entry

jeremymeng · 2021-01-15T01:02:39Z

/azp run js - storage-blob - tests

azure-pipelines · 2021-01-15T01:02:49Z

Azure Pipelines successfully started running 1 pipeline(s).

jeremymeng · 2021-01-15T18:31:04Z

Storage failures look unrelated. Same failures previously in master.

because a boolean value is not enough to determine whether a response is streaming or not. A list of streaming response status code `streamResponseStatusCodes` is introduced and used instead. ServiceClient no longer sets the deprecated `streamResponsebody` property based on incorrect assumption. However we still respect `streamResponseBody` in case it's set by users. Core v2 removes `streamResponseBody`.

…ream-body-when-error

jeremymeng · 2021-01-22T18:03:39Z

/azp run js - storage-blob - tests

azure-pipelines · 2021-01-22T18:03:51Z

Azure Pipelines successfully started running 1 pipeline(s).

as Blob.text() is not supported in IE.

var => const

jeremymeng · 2021-01-26T00:56:50Z

sdk/core/core-http/src/xhrHttpClient.ts

              xhr.addEventListener("load", () => {
-                resolve(xhr.response);
+                // Response comes back in Blob when xhr.responseType === "blob"


I had thought about always using xhr.responseType === "blob" to send request, then converting to text if needed based on the response status codes, but was back-pedaled because I don't want to make big changes to existing code paths.

xirzec

I like the design approach with status codes a lot! I think my only concern at this point is it feels like we're overly prioritizing the blob case in reading the result from XHR. Are we not able to detect the simple case at all?

xirzec · 2021-01-26T00:58:22Z

sdk/core/core-http/review/core-http.api.md

@@ -932,7 +932,9 @@ export class WebResource implements WebResourceLike {
    requestId: string;
    shouldDeserialize?: boolean | ((response: HttpOperationResponse) => boolean);
    spanOptions?: SpanOptions;
+    // @deprecated (undocumented)


why does it say (undocumented)?

likely needing a comments following @deprecated. I will add one pointing to the new property.

xirzec · 2021-01-26T01:01:06Z

sdk/core/core-https/karma.conf.js

@@ -100,7 +100,7 @@ module.exports = function(config) {

    // Continuous Integration mode
    // if true, Karma captures browsers, runs the tests and exits
-    singleRun: true,
+    singleRun: false,


was this intended to be changed? I'm assuming it was altered for testing.

What I usually do when I need to debug is

npx karma start --browsers=Chrome --single-run=false

We have --single-run in the unit-test:browser/integration-test:browser npm scripts so it's less to type when we need debug with just npx karma start --browsers=Chrome but I can revert since it's unrelated change.

I am cool with whatever is best here, I just thought it was a debug thing creeping into the main test path

xirzec · 2021-01-26T01:03:07Z

sdk/core/core-https/src/pipelineRequest.ts

@@ -130,7 +130,7 @@ class PipelineRequestImpl implements PipelineRequest {
    this.keepAlive = options.keepAlive ?? true;
    this.proxySettings = options.proxySettings;
    this.skipDecompressResponse = options.skipDecompressResponse ?? false;
-    this.streamResponseBody = options.streamResponseBody ?? false;
+    this.streamResponseStatusCodes = options.streamResponseStatusCodes ?? new Set<number>();


why initialize it to an empty set? Shouldn't we leave it undefined since it's optional?

Somehow I defined it as non-optional. Making it optional now.

xirzec · 2021-01-26T01:04:10Z

sdk/core/core-https/src/xhrHttpsClient.ts


    if (isReadableStream(request.body)) {
      throw new Error("Node streams are not supported in browser environment.");
    }

    xhr.send(request.body === undefined ? null : request.body);

-    if (request.streamResponseBody) {
+    if (request.streamResponseStatusCodes?.size ?? 0 > 0) {


Would this technically also work?

Suggested change

if (request.streamResponseStatusCodes?.size ?? 0 > 0) {

if (request.streamResponseStatusCodes?.size) {

Since I assume it can't be negative and null/undefined is falsy

xirzec · 2021-01-26T01:06:06Z

sdk/core/core-https/src/xhrHttpsClient.ts

+                    headers: parseHeaders(xhr)
+                  });
+                } else {
+                  // Blob.text() is not supported in IE


core-https doesn't support IE as a goal, so please remove this fallback.

xirzec · 2021-01-26T01:07:20Z

sdk/core/core-https/src/xhrHttpsClient.ts

      return new Promise((resolve, reject) => {
        xhr.addEventListener("readystatechange", () => {
          // Resolve as soon as headers are loaded
          if (xhr.readyState === XMLHttpRequest.HEADERS_RECEIVED) {
-            const blobBody = new Promise<Blob>((resolve, reject) => {
+            if (request.streamResponseStatusCodes?.has(xhr.status)) {
+              // eslint-disable-next-line @typescript-eslint/no-shadow


can avoid needing to disable lint by renaming resolve and reject here, probably good for readability, even if it's just to call them blobResolve and blobReject

There's another linting rule complaining about the params of Promise constructor callback must be named resolve and reject. The old code chose to use // eslint-disable-next-line @typescript-eslint/no-shadow.

The other error is: "Promise constructor parameters must be named resolve, reject promise/param-names"

I understand now

xirzec · 2021-01-26T01:07:45Z

sdk/core/core-https/src/xhrHttpsClient.ts

@@ -59,31 +59,63 @@ export class XhrHttpsClient implements HttpsClient {
    for (const [name, value] of request.headers) {
      xhr.setRequestHeader(name, value);
    }
-    xhr.responseType = request.streamResponseBody ? "blob" : "text";
+    xhr.responseType = request.streamResponseStatusCodes?.size ?? 0 > 0 ? "blob" : "text";


same thought as below

xirzec · 2021-01-26T01:10:47Z

sdk/core/core-https/src/xhrHttpsClient.ts

-                resolve(xhr.response);
+                // response comes back in Blob when xhr.responseType === "blob"
+                // but the response body type is not expected to be stream based on response status code
+                // so converting from Blob to text


I'm confused about this comment. How do we know the response is a blob and that we can't just read xhr.responseText? I don't see any actual detection logic, we're only asserting that this isn't a known streaming case.

I assume that our operation spec lists all streaming operations thus we don't need any more detection logic.

Before we send a request with xhr we set xhr.responseType to be either "blob" or "text" (default will be text if not specified). The response will come back in response, typed accordingly. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType

If the operation is streaming (based on our heuristic of any status code having a Stream body mapper) we use "blob" to send request. Otherwise we use "text".

The doc also mentions that "If the server returns data that is not compatible with the responseType that was set, the value of response will be null." However, in my testing, it still worked when we send a request using "blob" and Storage service return the error response in application/xml in response of Blob type.

So in the cases where we send requests for streaming operations and a response status code doesn't match any of the streaming status codes, the response has Blob type and needed to be converted into string.

EDITED: fixing typo

It turns out I was reading the code wrong. The if block was so big for if (streaming) that I didn't notice the else case (that was excluded from the diff view) where we use text in the non-blob case, so I thought we were treating every response as a blob.

Maybe as a refactoring we could move the blob case out into a helper function? That would also fix the nested-promise constructor naming collision issue.

Good idea! I extracted the "blob" case into a function.

jeremymeng · 2021-01-26T19:24:27Z

it feels like we're overly prioritizing the blob case in reading the result from XHR. Are we not able to detect the simple case at all?

I think the problem is xhr sets a response type before sending the request, and shapes the response accordingly. If we don't set response type it will defaults to text which is probably not working for streaming responses.

@deprecated

- Add @deprecated comment - Remove IE workaround from core v2.

xirzec

Everything looks good. Turns out I misread part of the code because I ran out of caffeine in my blood or something. I left a small comment reply on how we could make that code slightly more obvious, but it's not required.

- Extract Blob response case into a function - small cleanup

jeremymeng requested review from bterlson, chradek, daviwil and xirzec as code owners January 13, 2021 01:17

ghost added the Azure.Core label Jan 13, 2021

jeremymeng commented Jan 13, 2021

View reviewed changes

xirzec reviewed Jan 13, 2021

View reviewed changes

jeremymeng commented Jan 14, 2021

View reviewed changes

Better variable name

075e420

Change log entry

Azure deleted a comment from azure-pipelines bot Jan 15, 2021

jeremymeng mentioned this pull request Jan 15, 2021

[core v2] incorrect way to determine whether a response body is a stream or not #13235

Closed

jeremymeng added 2 commits January 20, 2021 13:43

Merge remote-tracking branch 'upstream/master' into corehttp-parse-st…

98e45eb

…ream-body-when-error

jeremymeng requested a review from ramya-rao-a as a code owner January 20, 2021 23:09

jeremymeng added 5 commits January 20, 2021 15:57

Fix build

8aeca4e

Merge remote-tracking branch 'upstream/master' into corehttp-parse-st…

e6e1c63

…ream-body-when-error

Fix merging issue

542d6e3

Merge remote-tracking branch 'upstream/master' into corehttp-parse-st…

1a52085

…ream-body-when-error

Fix build error after merging latest master

76fb967

jeremymeng requested review from HarshaNalluru, jiacfan, ljian3377, vinjiang and XiaoningLiu as code owners January 21, 2021 21:27

jeremymeng added 2 commits January 22, 2021 15:52

Use FileReader to convert Blob into string

09b6d47

as Blob.text() is not supported in IE.

Fixing linting errors

ab2e0fb

var => const

jeremymeng commented Jan 26, 2021

View reviewed changes

xirzec reviewed Jan 26, 2021

View reviewed changes

Address CR feedback

55d84f4

- Add @deprecated comment - Remove IE workaround from core v2.

xirzec approved these changes Jan 28, 2021

View reviewed changes

Refactoring

a9848af

- Extract Blob response case into a function - small cleanup

jeremymeng merged commit e140079 into Azure:master Jan 28, 2021

jeremymeng deleted the corehttp-parse-stream-body-when-error branch January 28, 2021 22:29

jeremymeng mentioned this pull request Apr 9, 2021

[Storage] Error code and message should live as top level properties on the error object #5796

Closed

		@@ -153,14 +154,20 @@ export abstract class FetchHttpClient implements HttpClient {
		const response: CommonResponse = await this.fetch(httpRequest.url, requestInit);

		const headers = parseHeaders(response.headers);

	if (request.streamResponseStatusCodes?.size ?? 0 > 0) {
	if (request.streamResponseStatusCodes?.size) {

[core-http] use response status to determine whether body is stream #13192

[core-http] use response status to determine whether body is stream #13192

Conversation

jeremymeng commented Jan 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng Jan 14, 2021 • edited

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

jeremymeng commented Jan 15, 2021

azure-pipelines bot commented Jan 15, 2021

jeremymeng commented Jan 15, 2021

jeremymeng commented Jan 22, 2021

azure-pipelines bot commented Jan 22, 2021

Choose a reason for hiding this comment

xirzec 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng commented Jan 26, 2021

xirzec left a comment

Choose a reason for hiding this comment

jeremymeng commented Jan 13, 2021 •

edited

jeremymeng Jan 14, 2021 •

edited

jeremymeng Jan 26, 2021 •

edited