yutakahirano / fetch-with-streams

Fetch API integrated with Streams.
66 stars 8 forks source link

Keep "null body" notion #39

Closed yutakahirano closed 7 years ago

yutakahirano commented 9 years ago

I merged "null body" and "empty body" in the draft, but it is not correct. We need to distinguish them because empty body has Content-Length while null body doesn't.

yutakahirano commented 9 years ago

Content-Length insertion only happens on Request, so Response is not related in terms of Content-Length. BodyUsed may be affected though.

tyoshino commented 9 years ago

As @annevk mentioned in https://github.com/whatwg/fetch/issues/61#issuecomment-114934302, we should make null body (no message-body) and empty body (empty message-body) distinguishable.

https://tools.ietf.org/html/rfc7230#section-3.3

The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field. Request message framing is independent of method semantics, even if the method does not define any use for a message body.


so Response is not related in terms of Content-Length.

When a Response is passed to event.respondWith() (https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#respond-with-method), it should recover the semantics (message-body exists or not).

So, at least the internal associated body should keep the signal. And, I think we should make .body also represent the difference.

yutakahirano commented 9 years ago

so Response is not related in terms of Content-Length.

When a Response is passed to event.respondWith() (https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#respond-with-method), it should recover the semantics (message-body exists or not).

So, at least the internal associated body should keep the signal. And, I think we should make .body also represent the difference.

That depends on the result of https://github.com/whatwg/fetch/issues/61. If we need to distinguish null / empty for bodyUsed, it could be natural to set null to body property. Otherwise, we can say that we don't have a Response with null body.

mnot commented 9 years ago

Note that we're going to have to revise the text quoted above to account for HTTP/2. Effectively, ALL requests have message bodies; it's just that some methods don't define semantics for them, so it's only sensible to send a 0-length body.

For responses, the relevant text in 7230 is a bit below that quoted above:

All 1xx (Informational), 204 (No Content), and 304 (Not Modified) responses do not include a message body. All other responses do include a message body, although the body might be of zero length.

This is hardwired into HTTP; there will be no future status codes that don't have a body.

domenic commented 9 years ago

Given that, it seems better to me to only reflect empty bodies in the fetch API. @annevk what do you think?

annevk commented 9 years ago

https://fetch.spec.whatwg.org/#concept-main-fetch:

If request's method is HEAD or CONNECT, or response's internal response's status is 101, 204, 205, or 304, set response's internal response's body to null and disregard any pushing toward it (if any).

It seems there is some mismatch between HTTP and implementations.

@domenic it seems fine to reflect null as empty in the API and not support null for requests (perhaps indeed model requests as not supporting null). Not entirely sure that solves any problems for us though and underneath it seems we still need to support the notion of a null body for responses.

domenic commented 9 years ago

@annevk what concrete mismatch are you referring to?

annevk commented 9 years ago

Well, e.g. 205 creates a null body in implementations, but not per HTTP...

domenic commented 9 years ago

I'm still curious what test code you're using to distinguish between null and empty bodies. @mnot assures me it's semantic only. What JS code are you using to expose the difference?

annevk commented 9 years ago

Oh, whether or not Content-Length is set works.

domenic commented 9 years ago

I'm not sure what Content-Type has to do with null body? Even if you meant Content-Length it still isn't really relevant. (A Content-Length header implies the presence of a body, but the absence of a Content-Length header does not imply that the body is null. It just means that the set of headers is different.)

domenic commented 9 years ago

Also still confused how this is a browser behavior; the server decides whether or not to send Content-* headers.

annevk commented 9 years ago

I guess you're right that for responses it depends. For requests the browser decides. It's not entirely clear to me what you're getting at.

domenic commented 9 years ago

I'm trying to correlate @mnot's assurances that this is a semantic-only difference with your claim that it has observable consequences. So far I'm still not seeing any observable consequences; that's what I'm getting at.

Even for requests, the only observable thing is what headers it has. In most HTTP libraries, this is entirely under the control of the person making the request. In the browser, maybe we want to enforce that either Content-Length or Transfer-Encoding: chunked is present. Or maybe we don't. But none of this has anything to do with the notion of null bodies vs. empty bodies.

Let's put it this way. Say we totally removed the notion of null bodies from the spec. What observable changes are there from JS? I cannot find any, based on the current spec. You cannot write a server that vends /null-body and /empty-body and JS can tell the difference; similarly, I am pretty sure you cannot write a server such that I can send two different Request objects to it, and it can tell that in one case I meant to send a semantically null body, and in the other I meant to send a semantically empty body.

(And if there is some observable difference that I missed, I am betting it is better expressed in terms of customizability headers, not in terms of null vs. empty body.)

annevk commented 9 years ago

In the browser, maybe we want to enforce that either Content-Length or Transfer-Encoding: chunked is present.

We do. And we enforce Content-Length since that is what everyone implements.

But none of this has anything to do with the notion of null bodies vs. empty bodies.

Why not? So far Content-Length: 0 != missing Content-Length which means that there could be content relying on that distinction (seems unlikely, but until browsers are actually converged to always emit Content-Length: 0 we don't know).

tyoshino commented 9 years ago

So, given the current situation (someone relying on the distinction), we choose to have an option to indicate no message-body as null body to the UA but leave the decision whether to converge to the latest RFC's recommendation or not up to the UA?

https://fetch.spec.whatwg.org/#dom-request

  1. If either init's body member is present or temporaryBody is non-null, and request's method is GET or HEAD, throw a TypeError.

This is already placing a stronger requirement on GET and HEAD than RFC7230.

https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch

If request's body is null and request's method is HEAD, POST, or PUT, set contentLengthValue to 0.

This is already aligned with @mnot's plan. Other methods are not covered, but the plan is that we'll leave this as-is to allow issuing them w/o message-body?


And, no change on HTTP network fetch. Keep considering that absence of Content-Length/Transfer-Encoding gets translated into response's body being null?

domenic commented 9 years ago

We have to separate requests and responses. Per spec:

IMO it would be simpler if req.body and res.body were never null. If someone wanted to write perSpecIsBodyNull, they would check the status code and/or headers, and also switch on which HTTP version they were implementing. But nobody would really want to do that, except someone writing some kind of HTTP semantics tool.

This simpler idea does separate req/res.body === null or not from "per spec request/response body is null", but that seems fine. The fact that both HTTP and JavaScript have the term "null" is just a coincidence.

annevk commented 9 years ago

I feel like we are conflating a ton of issues:

  1. Can we change user agents to always have a Content-Length for requests? (Including GET?)
  2. Can we change the JavaScript API to never return null from the body getter? (Of course.)
  3. Why is there a mismatch between HTTP and Fetch (e.g. the latter considers 205 as resulting in a null body too)?
  4. Probably more?
domenic commented 9 years ago

Agreed! I think the most interesting mismatch is indeed 3. Why did Fetch invent its own concept of null body? And, does Fetch having this concept of null body add any value to Fetch?

My general tendency is to make the internal concepts match the external JS APIs as much as possible. Thus, since I think having the body getter always return a body is nice, I also think it would be nice if Fetch did not have any concept of null body.

There can be a separate concept, called "does this request or response have a Content-Length header." The rules for that should probably be:

The "does this request or response have a Content-Length header" concept can of course map exactly to the question of req/res.headers.has("Content-Length").

annevk commented 9 years ago

Fetch has a concept of a null body because implementations do. It seems though that you are splitting 3, because:

A. Both HTTP and Fetch have a null body concept. You wonder why. B. HTTP and Fetch have it for different things. You don't seem to care about this, whereas that was mostly what I was getting at.

My ongoing theory in standards development is that if implementations have an internal concept, it will get exposed at some point. So if we're going to drop it from the model, we need to be very confident it can be removed from implementations as well (or at least never be exposed in one way or another).

And no, you cannot map it to has("Content-Length") since request's won't have that header until just before they hit the cache/network.

mnot commented 9 years ago

@annevk - is fetch's assumption that 205 has a null body based upon testing current implementations?

annevk commented 9 years ago

@mnot yeah, e.g. https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#1490. In particular, https://github.com/whatwg/fetch/issues/31 is what made me add that text.

yutakahirano commented 9 years ago

My ongoing theory in standards development is that if implementations have an internal concept, it will get exposed at some point. So if we're going to drop it from the model, we need to be very confident it can be removed from implementations as well (or at least never be exposed in one way or another).

I generally like conflating null and empty if there is no use-case for distinguishing them. Checking nullity every time is tiresome.

However, I am not 100% sure that there will never be such a use-case.

mnot commented 9 years ago

So, it seems like this is really about 205; the only evidence we have (so far) is that mozilla enforces a 0-length body on that status code.

Note that doing so is not against the recommendations in RFC7231; it says this about 205:

Since the 205 status code implies that no additional content will be provided, a server MUST NOT generate a payload in a 205 response. In other words, a server MUST do one of the following for a 205 response: a) indicate a zero-length body for the response by including a Content-Length header field with a value of 0; b) indicate a zero-length payload for the response by including a Transfer-Encoding header field with a value of chunked and a message body consisting of a single chunk of zero-length; or, c) close the connection immediately after sending the blank line terminating the header section.

http://httpwg.github.io/specs/rfc7231.html#rfc.section.6.3.6

Where there's a divergence is when 205 contains a body despite this advice; 7231 says that the 205 has a forced zero-length body, but is specified in such a way that if it does have a body, the parser can still recover and continue to use the connection for the next message.

That said, I'd be a little surprised if Mozilla didn't have error recovery for the case when 304 and friends have a body, because they sometimes do on the wild Web, and HTTP allows you to do error recovery.

@mcmanus, any insight?

mcmanus commented 9 years ago

That said, I'd be a little surprised if Mozilla didn't have error recovery for the case when 304 and friends have a body, because they sometimes do on the wild Web, and HTTP allows you to do error recovery.

The channel caller for 304/204/205 responses will always get a 0 byte body - we just parse the response as if Content-Length 0 were set. There are heuristics to skip over the rest of the junk that isn't supposed to be there if it should show up, but the content won't see it.

annevk commented 9 years ago

So we're keeping the null concept in Fetch because empty stream is really maybe empty stream (you cannot know until you read). We should have note explaining what null, maybe empty stream means in terms of Fetch, Streams, and HTTP.

domenic commented 9 years ago

To follow-up. From JS's perspective, request/response's body getter should return null when:

Per https://github.com/whatwg/fetch/issues/86, the above imply that whenever the status code of a Response is 101, 204, 205, or 304, body will return null (since otherwise the constructor would have thrown).

Per the current spec, any Responses with method GET or HEAD, will have a body getter returning null (since otherwise it would have thrown during construction).


We will then need a nice informative note explaining what req/res.body === null means in the Fetch API. Namely, it means known-empty body, whereas it's also possible to have possibly-empty bodies via streams. We'll also want to clarify how this JS null does not relate to HTTP's semantic concept of null or empty body.

yutakahirano commented 9 years ago

Done at https://github.com/yutakahirano/fetch-with-streams/commit/b2dd9a5bb1a75efb44846fe0c00fa83faac241b1 for Response. I would like to keep this issue for Request, but it is out of the first release scope.

annevk commented 7 years ago

This can be closed I think.