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

Change fetch description to match real world experience #27927

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Conversation

fozcode
Copy link
Contributor

@fozcode fozcode commented Jul 13, 2023

Description

Attempt to better describe the behaviour of fetch.

Motivation

The previous wording was misleading because fetch does in fact reject for some HTTP status codes.

Additional details

Documents the behaviour seen today in Chrome 114.0.5735.198

Related issues and pull requests

@fozcode fozcode requested a review from a team as a code owner July 13, 2023 16:14
@fozcode fozcode requested review from sideshowbarker and removed request for a team July 13, 2023 16:14
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jul 13, 2023
@Josh-Cena
Copy link
Member

Josh-Cena commented Jul 13, 2023

Does it reject on 400? See #17115. We would probably need to be comprehensive here about what statuses lead to rejection.

@fozcode
Copy link
Contributor Author

fozcode commented Jul 14, 2023

The situation appears to be complicated.

For my test case, in Chrome:

Status code Rejects
400 no
401 no
402 no
403 no
404 no
405 no
406 no
407 yes
408 no
413 no
421 no
431 no

Meanwhile Firefox does not reject for 407.

So the rejection for 407 in Chrome is arguably a Chrome bug, or perhaps it can be said that the result is implementation dependent.

@fozcode
Copy link
Contributor Author

fozcode commented Jul 14, 2023

I probably should have consulted Stack Overflow earlier but it seems this is not a new situation:
https://stackoverflow.com/questions/60753827/fetch-api-handle-error-407-is-not-same-between-chrome-and-firefox

@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2023

Preview URLs

(comment last updated: 2024-03-18 17:08:35)

@sideshowbarker
Copy link
Collaborator

We need to document what the spec says the required behavior is, and add notes indicating what browsers don’t conform to the behavior the spec requires.

https://fetch.spec.whatwg.org/#ref-for-concept-response-status①⑧ is the part of the spec that defines how browsers are required to handle 407 errors.

@annevk
Copy link
Contributor

annevk commented Jul 14, 2023

This would probably benefit from some web-platform-tests so we can analyze the differences. In any event the specification does not require rejection. It requires handling 407 by the user agent, which is not very different from it requiring handling 301.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jul 17, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@sideshowbarker sideshowbarker removed their request for review November 11, 2023 06:39
@bsmth
Copy link
Member

bsmth commented Mar 15, 2024

Thanks for raising this one @fozcode.

I looked through the Chromium bug tracker and there's a few related to this behavior, but this seems to explain what's going on: https://issues.chromium.org/issues/41401857

When we get a 407, we immediately try to handle it in http_network_transaction. When that fails, we return ERR_UNEXPECTED_PROXY_AUTH. This is probably the right thing to do in general, but per the XHR spec, we're supposed to read the body and return everything as a successful XHR response.

So this is Chrome-only and it looks like it should be a note in BCD (in 407 response?) with a link to https://issues.chromium.org/issues/41401857

(related: https://groups.google.com/a/chromium.org/g/chromium-bugs/c/JnTQ5DWyE4A/m/RT18gr_sdLMJ)

I can also suggest we close this PR to leave the prose as-is, what do you think?

@fozcode
Copy link
Contributor Author

fozcode commented Mar 15, 2024

Thanks for reviewing this @bsmth. I didn't realise the situation was specific to 407 in Chrome when I raised the PR. Given that it is, I have no objections to closing this.

Though it is now off-topic, the original text says that a rejection due to a network error is "usually when there's a permissions issue". That seems oddly specific in my opinion, when it might be that the destination is unreachable for a dozen other reasons.

@bsmth
Copy link
Member

bsmth commented Mar 18, 2024

Thanks for reviewing this @bsmth. I didn't realise the situation was specific to 407 in Chrome when I raised the PR. Given that it is, I have no objections to closing this.

Though it is now off-topic, the original text says that a rejection due to a network error is "usually when there's a permissions issue". That seems oddly specific in my opinion, when it might be that the destination is unreachable for a dozen other reasons.

Yes, I agree it's not super clear. I've left a suggestion above, what do you think? We may try to clean it up here and rename the PR title to reflect that.

Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
@github-actions github-actions bot added the size/s [PR only] 6-50 LoC changed label Mar 18, 2024
@fozcode
Copy link
Contributor Author

fozcode commented Mar 18, 2024

I've accepted the suggestion. Versus the original text it is clearer that in one situation there is no response, while in the other there is a response but it's an error.

@github-actions github-actions bot added size/xs [PR only] 0-5 LoC changed and removed merge conflicts 🚧 [PR only] size/s [PR only] 6-50 LoC changed labels Mar 18, 2024
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Super, thank you very much. I think we can leave the PR title as-is and merge! We also need a BCD issue to track the 407 behavior on Chrome as a follow-up (I can do this).

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/xs [PR only] 0-5 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants