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

421 and upload streams #982

Closed
annevk opened this issue Dec 12, 2019 · 27 comments
Closed

421 and upload streams #982

annevk opened this issue Dec 12, 2019 · 27 comments

Comments

@annevk
Copy link
Member

annevk commented Dec 12, 2019

Per #497 browsers implement 421. What does a 421 mean for an upload stream? Would we reveal the 421 to the caller after streaming completed?

Would clients that support early responses cancel the stream in addition?

cc @whatwg/http @yutakahirano

@yoichio
Copy link
Contributor

yoichio commented Jul 20, 2020

Since ReadableStream is not replayable, 421 handling should be referred to the web author.

I guess supporting early responses cancel is hard as same as bidirectional support given Chromium starts response handling after whole request body sending.
cc @MattMenke2

@MattMenke2
Copy link
Contributor

MattMenke2 commented Jul 20, 2020

I'm no expert on the mechanics of sockets, but if we write and the socket is half-closed by the other end, without reading from it, my recollection is we tend to get connection resets, as opposed connection closed notifications. This behavior is observed across all platforms.

Once Windows notifies a consumer of a connection reset, you can no longer read from the socket, even if there was a 421 response pending. So just trying to read from a socket after uploading the request body fails, which would be much simpler than supporting full duplex sockets, won't give us a 421 response on Windows. On other platforms, we do actually read from the body on connection resets of upload attempts, and return 4xx/5xx responses, if we get them.

Note that this only applies to H1, I'm not sure what H2/QUIC do in this case, or if they even have an analog of half-closed sockets.

@yoichio
Copy link
Contributor

yoichio commented Jul 30, 2020

Ping @annevk ?

@annevk
Copy link
Member Author

annevk commented Aug 4, 2020

I don't really have a stake in the outcome, but when we pass it on to the web developer (and when we retry) needs to be clear.

@yoichio
Copy link
Contributor

yoichio commented Aug 5, 2020

If fetch upload failed, promise reject is called. If it succeeded and server returns 421, the web author gets 421.

@annevk
Copy link
Member Author

annevk commented Aug 5, 2020

I meant in the specification. 😊

@yoichio
Copy link
Contributor

yoichio commented Aug 5, 2020

I don't understand yet what is special and unclear about 421 specification and the upload stream.
https://tools.ietf.org/html/rfc7540#section-9.1.2

@annevk
Copy link
Member Author

annevk commented Aug 6, 2020

Well, the request cannot be retried and therefore the 421 is forwarded to the application, whereas in other cases retrying is allowed. (I haven't tested what browsers do with it in general though.)

@yoichio
Copy link
Contributor

yoichio commented Aug 12, 2020

I confirmed that both Chrome and Firefox retried the request once upon the 421 response for simple string fetch except
only Chrome prompted POST <url> 421 (Misdirected Request) error in devtool.

As fetch() fails on redirect resolution, how about
specifying "fetch() fails with the promise-error "Browser tried to retry the request on 421 but Stream can not be replayed" " as well?
or
As the IETF spec states "Clients receiving a 421 (Misdirected Request) response from a server MAY retry the request", not retrying
anything is also valid behavior. So no spec update is needed?

@annevk
Copy link
Member Author

annevk commented Aug 14, 2020

I think we should require retrying (similar to how we require redirects to be followed, etc.) and forbid it when a request body is a stream.

@yoichio
Copy link
Contributor

yoichio commented Aug 18, 2020

I agree with not retrying when a request body is a stream.
I'm not sure if we should change the spec from "MAY" to "SHOULD" about other body's.

@annevk
Copy link
Member Author

annevk commented Aug 18, 2020

I'm saying MUST. Fetch is defining a type of client after all. I suppose we could make it less strict initially, but if every browser already does it I don't see why we would.

@yoichio
Copy link
Contributor

yoichio commented Aug 19, 2020

Discussing the relation between 421 response and other body fetching is out of this issue blocking upload-streaming.

@yoichio
Copy link
Contributor

yoichio commented Oct 2, 2020

Ping?

@annevk
Copy link
Member Author

annevk commented Oct 5, 2020

@yoichio ping for what?

@yoichio
Copy link
Contributor

yoichio commented Oct 6, 2020

The current spec states "Clients receiving a 421 (Misdirected Request) response from a server MAY retry the request".
You're suggesting "___ MUST retry the request if the body is not streaming", right?

@annevk
Copy link
Member Author

annevk commented Oct 6, 2020

Right, because Fetch defines a particular type of client that ideally all behave the same.

@yoichio
Copy link
Contributor

yoichio commented Oct 7, 2020

I overlooked the auxiliary verbs meaning: MAY is absolutely optional.

If changing the MAY to MUST is required to mean we must not retry streams body, I agree with you.

@yoichio
Copy link
Contributor

yoichio commented Oct 21, 2020

Anne, what do you think of my above comment?

@annevk
Copy link
Member Author

annevk commented Oct 21, 2020

I'm not entirely sure what it means. What I think needs to happen is that Fetch mandates a particular behavior in response to 421 responses for all types of requests Fetch can make.

@yoichio
Copy link
Contributor

yoichio commented Oct 22, 2020

Fetch mandates a particular behavior in response to 421 responses for all types of requests Fetch can make.

A particular behavior means a same action?
As we're discussing at #538, fetch doesn't retry if the body is stream to 30X responses.

@annevk
Copy link
Member Author

annevk commented Oct 22, 2020

Sorry, I did phrase that awkwardly. I meant that we should specify how to react to 421 responses for all requests. And "how" can be different depending on the type of request. E.g., if request's body is a stream, we return the response. If it's not, we refetch. (Similar indeed to how the specification details how to deal with 1XX and 304 for instance.)

@yoichio
Copy link
Contributor

yoichio commented Oct 26, 2020

Thanks for clarifying that.

I'm not familiar with the spec grammar and writing.
How about rewriting the current spec with

If the body is not streaming, clients receiving a 421 (Misdirected Request) response from a server MUSTY retry the request.
If the body is streaming, clients receiving a 421 (Misdirected Request) response from a server MUSTY not retry the request.

?
and which spec (rfc7540 or whatwg/fetch) should be updated?

@annevk
Copy link
Member Author

annevk commented Oct 26, 2020

This would have to be in Fetch as browser considerations for 421 are not universal. I suspect it would be easiest for you to reach out to @yutakahirano for advice on how to go about it.

@yutakahirano
Copy link
Member

@yoichio, last year we discussed this and IIRC you were willing to handle this issue, so I assigned this issue to you.

Please let me know if you need any help.

@yoichio
Copy link
Contributor

yoichio commented Jan 19, 2021

Yes. I'll pull-request.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 22, 2021
Following whatwg/fetch#982,
fetch upload streaming on 421 (Misdirected Request) should be rejected.

Why this results with PASS though we don't have an implementation yet is
because the uploading always fails over H/1
(see crrev.com/c/2174099 for detail).

Bug: 688906
Change-Id: I4cb02663f0c8294fe25d675b60978b4a7fcbc749
@annevk
Copy link
Member Author

annevk commented Mar 5, 2021

Closed by c5eb621 (#1141).

@annevk annevk closed this as completed Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants
@annevk @yutakahirano @yoichio @MattMenke2 and others