Skip to content

Fix issue 13208: document response body access exceptions#34576

Merged
sideshowbarker merged 4 commits intomdn:mainfrom
wbamberg:fix-13208
Jul 4, 2024
Merged

Fix issue 13208: document response body access exceptions#34576
sideshowbarker merged 4 commits intomdn:mainfrom
wbamberg:fix-13208

Conversation

@wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Jul 3, 2024

Fixes #13208.
Fixes #24466 and closes #31707.
Fixes #12387.

@wbamberg wbamberg requested a review from a team as a code owner July 3, 2024 00:09
@wbamberg wbamberg requested review from sideshowbarker and removed request for a team July 3, 2024 00:09
@github-actions github-actions bot added Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed labels Jul 3, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

@Josh-Cena
Copy link
Member

How does this relate to #31707 and #24466?

@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 3, 2024

How does this relate to #31707 and #24466?

It fixes #24466 and closes #31707.

@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 3, 2024

@sideshowbarker
Copy link
Member

sideshowbarker commented Jul 3, 2024

Although there might be some additional exceptions, given stackoverflow.com/questions/49785911/is-it-possible-to-cause-a-fetchs-text-function-to-throw/49786130 .

I see that says this:

text may throw

  • if its request is aborted between the completion and the consumption

As far as I can see, that wouldn’t at all be a case of text() throwing — because text() operates on the response body, after the browser has has successfully made the request, and after the browser has received the response. Calling text() doesn’t cause the browser to initiate a new request.

if the Content-Encoding response header doesn't match the corresponding entity-body

OK, that one seems basically right.

It appears to be based on whatwg/fetch#657 and whatwg/fetch#710, with related tests in http://wpt.live/fetch/content-encoding — for example, http://wpt.live/fetch/content-encoding/gzip/bad-gzip-body.any.html

There are open browser bugs for it at https://bugs.webkit.org/show_bug.cgi?id=184882 and https://bugzilla.mozilla.org/show_bug.cgi?id=1456105 — but it’s not clear to me what else needs to be done on those, since the tests I tried all passed.

Anyway, the relevant exception isn’t specific to text(); it also gets thrown for any of the Body methods: arrayBuffer(), blob(), formData(), json(), too.

As far as what it should say, I think it should just be:

- {{jsxref("TypeError")}}
  - A `Content-Encoding` decoding failure has occurred.

…that is, rather than something like what that SO answer says — “if the Content-Encoding response header doesn't match the corresponding entity-body”, which is too specific to cover all the possible decoding-failure cases.

Copy link
Member

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

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

This looks great to me. I don’t think it needs to be blocked on adding anything for the additional case I commented about in #34576 (comment) — I think that could be handled in follow-up PR if you wanted. Or if you wanted to add it here instead, I’d be happy to review it here too.

@Josh-Cena
Copy link
Member

because text() operates on the response body, after the browser has has successfully made the request, and after the browser has received the response. Calling text() doesn’t cause the browser to initiate a new request.

That is talking about abort() being called after the request promise has fulfilled but before the text() promise has fulfilled, I think. See also #12387

@sideshowbarker
Copy link
Member

sideshowbarker commented Jul 3, 2024

That is talking about abort() being called after the request promise has fulfilled but before the text() promise has fulfilled, I think. See also #12387

aha, thanks — so that would also apply to all other Body methods too. I guess something like this:

- {{jsxref("DOMException")}}
  - [AbortController: abort()](/en-US/docs/Web/API/AbortController/abort) is called
    before the promise returned by the method fulfills. 

@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 3, 2024

Yes, I can confirm that with code like this in the console:

async function get() {
    const controller = new AbortController();
    const url = "https://httpbin.org/get";
    const req = new Request(url, {
        signal: controller.signal
    })
    const resp = await fetch(req);
    controller.abort();
    console.log("aborted...")
    const text = await resp.text();
    console.log(text);
}

@Josh-Cena
Copy link
Member

OK, maybe we should fix one more issue in this PR then ^^ (Also needs one-sentence addition to AbortController but that should be it)

@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 3, 2024

I can reproduce the content decoding error with code like this:

async function f() {
	try {
		const request = new Request(
			"https://httpbin.org/response-headers?Content-Encoding=gzip"
		);
		const resp = await fetch(request);
		console.log(resp);
		console.log(`Content-Encoding:${resp.headers.get("content-encoding")}`);
                // Content-Encoding:gzip
		const text = await resp.arrayBuffer(); // Throws here
		console.log(text);
	} catch (e) {
		console.log(e);
                // TypeError: Decoding failed.
	}
}

@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Jul 3, 2024
@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 3, 2024

OK, I added 2 new exceptions to each of these methods:

- {{domxref("DOMException")}} `AbortError`
  - : The request was [aborted](/en-US/docs/Web/API/Fetch_API/Using_Fetch#canceling_a_request).
- {{jsxref("TypeError")}}
  - : There is an error decoding the body content (for example, because the {{httpheader("Content-Encoding")}} header is incorrect).

...so I chose a slightly different text from suggested, mostly because I thought "Content-Encoding decoding" sounded weird.

For #12387 I added a bit in "Using Fetch" about this, and also updated https://developer.mozilla.org/en-US/docs/Web/API/AbortController and https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal. I don't like documenting this in the last two places, for a couple of reasons: (1) that is in examples, which should not introduce new concepts, ever, and (2) this is an aspect of the Fetch API, not the AbortController API. But I'm not going to try to untangle that in this PR.

@wbamberg wbamberg requested a review from sideshowbarker July 3, 2024 23:20
@sideshowbarker sideshowbarker merged commit 889fd7c into mdn:main Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed

Projects

None yet

3 participants