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

Clarify semantics of checking done, closed, etc. states of a null body stream #635

Closed
harrishancock opened this issue Nov 20, 2017 · 14 comments

Comments

@harrishancock
Copy link
Contributor

harrishancock commented Nov 20, 2017

I'm trying to figure out the semantics of checking the done, closed, errored, disturbed statuses of a body's stream, when that stream is null.

Example 1: Non-303 redirect causes body done check

For an example of where a "null body stream done check" can occur, say we're in a service worker script making a fetch() request to a same-origin URL. Suppose that request has a body with or without a source buffer.

let request = new Request(someUrl, { method: 'POST', body: someBodyInitializer })
let response = await fetch(request)

The fetch request's body will initially possess a stream (and possibly a source, irrelevant to this example). By my reading, the stream will be nullified in step 2.2.4 of HTTP network-or-cache fetch, which is called from HTTP fetch step 4.3.

Say the response is a non-303 redirect. HTTP fetch step 5.1 kicks in and checks whether the request's body is done, i.e., "body is null or body’s stream is closed or errored." Since the request's body is not null, we must check if body's stream (which is null) is closed or errored, i.e., (null.[[closed]] || null.[[errored]]). I presume this would throw an error.

Example 2: Checking request.bodyUsed after fetch()

[Edit: This example, and the next, deal with perceived inconsistencies in request.bodyUsed after a call to fetch(request). As @yutakahirano pointed out, however, there is no problem here, because fetch(request) will copy the request's body, leaving the original request's body's stream disturbed, meaning request.bodyUsed's behavior is correct.]

Assume the same situation as above, but say we receive a non-redirect response. The fetch() will complete with the body's stream remaining null (from step 2.2.4 of HTTP network-or-cache fetch). Now say the user checks request.bodyUsed after the await fetch(request) completes. Since request's body itself is non-null, the bodyUsed accessor checks if body's stream is disturbed, defined as "IsReadableStreamDisturbed(stream) is true". But body's stream being null violates IsReadableStreamDisturbed()'s precondition (https://streams.spec.whatwg.org/#is-readable-stream-disturbed). I presume this would also throw an error.

A related issue: bodyUsed is false after 301, 302, 303 redirect

For a second, related issue, consider what happens in the above examples when the response is a 301 or 302. Since the request was a POST, the HTTP redirect-fetch algorithm will set request's method to GET and request's body to null in step 9 before passing it off to a recursive call to main fetch. Assume the subsequent GET request completes without incident and the user checks request.bodyUsed after the await fetch(request) line completes. Since request's entire body is null, it cannot be disturbed, so bodyUsed returns false. That seems counterintuitive to me -- the request initially had a body, it was transmitted in the first request, then the request was converted to a GET with a null body when the fetch algorithm hit a redirect. I would expect that bodyUsed should return true in this situation.

Note that this situation would also happen for a 303 redirect, assuming some workaround for the "null body stream done check" issue.

I can open a separate issue for this question, but wanted to mention it here, so I could explain it using the same example.

@annevk
Copy link
Member

annevk commented Nov 21, 2017

@yutakahirano can you look at this please?

@yutakahirano
Copy link
Member

Example 1: You're right, but I'm not sure how to fix it.

Example 2: fetch(request) creates a new Request object (see the second step of https://fetch.spec.whatwg.org/#fetch-method). As in the 43rd step in https://fetch.spec.whatwg.org/#dom-request, the newly created Request object will use a newly created ReadableStream which is associated with the original stream. Mutating the latter request will not affect the original request.

@annevk
Copy link
Member

annevk commented Nov 21, 2017

@yutakahirano can't we say that body is done when body's stream is null as well? I wonder if we should reconsider how we describe request bodies though as it's getting rather complicated.

@yutakahirano
Copy link
Member

I guess the step is to abort the stream when the body transmission is not done, right? If so, I think the user agent may send RST_STREAM in that case as well. Is it reasonable to listen to https://fetch.spec.whatwg.org/#process-request-end-of-body ?

@annevk
Copy link
Member

annevk commented Nov 21, 2017

Step 5.1 of https://fetch.spec.whatwg.org/#concept-http-fetch exists to stop sending the request body when there is a redirect as redirect endpoints have no use for it (except 303 endpoints, which might).

In case you follow that redirect and the request body is a stream, step 9 of https://fetch.spec.whatwg.org/#concept-http-redirect-fetch would return a network error as we don't support "restreaming".

I don't understand your question about process request end-of-body.

@yutakahirano
Copy link
Member

Even when we pass a non-streamable object (String/ArrayBuffer/etc) to the Request constructor, that object is converted to a ReadableStream. That means whenever we go through Step 4 of https://fetch.spec.whatwg.org/#concept-http-fetch we will see a null stream at Step 5.1.

@annevk
Copy link
Member

annevk commented Nov 21, 2017

That would mean those cannot follow redirects either, which would be wrong. We probably have to rethink how we define bodies then to keep streams a special case.

@harrishancock
Copy link
Contributor Author

harrishancock commented Nov 21, 2017

As in the 43rd step in https://fetch.spec.whatwg.org/#dom-request, the newly created Request object will use a newly created ReadableStream which is associated with the original stream. Mutating the latter request will not affect the original request.

Ah, right! Thanks for explaining this. There is no problem in the second/third examples, then -- I've edited my issue report to strike out those two headings, and added an explanatory note.

I guess that leaves the null body stream check in section 4.3 (HTTP fetch), step 5.1. Perhaps a better description for this issue might be, "Reconcile body stream nullification in HTTP network-or-cache fetch step 2.2.4 with body transmission interruption in HTTP fetch step 5.1."

Perhaps a request's body's stream should not be nullable at all, or at least not be nullified by the HTTP network-or-cache fetch algorithm? Its nullity never seems to be checked, so the HTTP-redirect fetch algorithm wouldn't change in behavior, but the body transmission interruption step (HTTP fetch 5.1) would start working.

@yutakahirano
Copy link
Member

@annevk, what happens if we send an RST_STREAM frame when the body is done?

@annevk
Copy link
Member

annevk commented Nov 24, 2017

Nothing harmful I believe. It just indicates the stream isn't needed.

@yutakahirano
Copy link
Member

Then can't we say something like

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.

?

@annevk
Copy link
Member

annevk commented Nov 24, 2017

That's probably fine, yes.

I'm still a bit confused about converting all bodies to streams, but I think remember now that it works because we also store the source and can therefore create endless streams rather than just the one. That's how that's supposed to work, right?

@yutakahirano
Copy link
Member

Yes.

annevk added a commit that referenced this issue Nov 24, 2017
Checking if body is not done had a problem in that if body's stream is null it would end up not working as "done" tries to read internal slots from a body's stream.

This also makes the language around RST_STREAM frames consistent.

Fixes #635.
@annevk
Copy link
Member

annevk commented Nov 24, 2017

Created #638 to address this.

annevk added a commit that referenced this issue Nov 27, 2017
Checking if body is not done had a problem in that if body's stream is null it would end up not working as "done" tries to read internal slots from a body's stream.

This also makes the language around RST_STREAM frames consistent.

Fixes #635.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants