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

redirect doesn't cancel or consume current response #1288

Open
ronag opened this issue Aug 10, 2021 · 9 comments
Open

redirect doesn't cancel or consume current response #1288

ronag opened this issue Aug 10, 2021 · 9 comments
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: http topic: redirects

Comments

@ronag
Copy link
Contributor

ronag commented Aug 10, 2021

Even though a server shouldn't send a body with a redirect response, it is still possible. The way the spec is defined we have a deadlock here if the connection is re-used where the redirect response body is waiting to be consumed but we discard the reference and start a new request (mainFetch). We should probably consume/cancel/abort current response body before calling mainFetch again.

We avoid the same issue with 427 by setting forceNewConnection in httpNetworkOrCacheFetch.

@annevk
Copy link
Member

annevk commented Aug 11, 2021

I'm not sure how much we want to say about this, though perhaps we should add some wording. A lot of this depends on the HTTP version.

@ronag
Copy link
Contributor Author

ronag commented Aug 11, 2021

I think you want to cancel/end/abort the response stream in some way regardless of http version? Maybe just write "cancel response stream" and leave what that means as an implementation detail?

@annevk
Copy link
Member

annevk commented Aug 11, 2021

Step 8 of https://fetch.spec.whatwg.org/#http-fetch does something like that. #637 is about making that version-agnostic.

This request is slightly different I suppose in that you are asking for mandatory behavior which is not unreasonable. We'd need everyone to agree on what that behavior should be though.

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest topic: http topic: redirects labels Aug 11, 2021
@ronag
Copy link
Contributor Author

ronag commented Aug 11, 2021

Ah, you mean:

If actualResponse’s status is not 303, request’s body is not null, and the connection uses HTTP/2, then user agents may, and are even encouraged to, transmit an RST_STREAM frame.

Not sure why the "status is not 303" pre-condition exists? A 303 response may also contain a body if the server doesn't follow spec.

@ronag
Copy link
Contributor Author

ronag commented Aug 11, 2021

In terms of "needs implementer interest" I can add Node to that list. I'm working on implementing fetch for nodejs through https://github.com/nodejs/undici.

@domenic
Copy link
Member

domenic commented Aug 11, 2021

The WHATWG working mode is about implementing specs in browser engines, so we need multiple browser engines interested in such a thing.

@ronag
Copy link
Contributor Author

ronag commented Aug 11, 2021

I see. Was hoping Node could be included in the list as an "engine". I assume then that WHATWG has limited (if any) involvement in Node.

@domenic
Copy link
Member

domenic commented Aug 11, 2021

Yeah, you can learn more about the WHATWG here: https://whatwg.org/faq#what-is-the-whatwg

@ronag
Copy link
Contributor Author

ronag commented Aug 11, 2021

Likewise the 407 case does not cleanup the active response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: http topic: redirects
Development

No branches or pull requests

3 participants