problem with .text() drain behavior and .bodyUsed

[wanderview]

Over here I raised a concern that the .bodyUsed flag was not getting set properly for .text().

#25 (comment)

Can someone explain how .text() can not set the .bodyUsed without requiring the browser to cache the entire contents of the stream in case someone calls it again?

And if the answer is "the code will hit the locked reader", it seems we still can't just break the meaning of bodyUsed on a shipped spec.

[yutakahirano]

At there I replied that it was discussed at w3c/ServiceWorker#452. Do you want to discuss it again? From which point?

[wanderview]

I'm sorry, but that thread is so long I'm having trouble determining what the outcome was.

My immediate concern is from this pull request:

web-platform-tests/wpt#1782

The web-platform-test was previously doing:

    return response.text().then(function() {
      assert_false(
        response.bodyUsed,
        '[https://fetch.spec.whatwg.org/#concept-body-consume-body] ' +
          'The text() method should not set "body passed" flag.');
      return cache.put(new Request(test_url), response);
    });

When I went to fix this I was told the fetch-with-streams spec was changing to this behavior:

https://critic.hoppipolla.co.uk/showcomment?chain=11816

I strongly object to this because its a complete reversal of the behavior currently shipping in fetch in both chrome and firefox. Also, it just seems completely broken from a webdev point of view. I just drained the Response but its body isn't used?

I think whatever fetch-with-streams does, there should be no observable difference to code using the existing .text()/.json()/etc in current fetch.

[yutakahirano]

Cc-ing discussion participants:
@annevk @domenic @tyoshino @horo-t

It is true that fetch-with-streams draft changes the behavior.

I strongly object to this because its a complete reversal of the behavior currently shipping in fetch in both chrome and firefox. Also, it just seems completely broken from a webdev point of view. I just drained the Response but its body isn't used?

So your opinion is that .bodyUsed should return if (part of) data is consumed by someone, right? Actually the current .bodyUsed in the fetch spec turns on even when text() reads nothing, i.e. text() called on an empty response. I don't like the behavior, at least.

[annevk]

I don't like the behavior, at least.

It makes the API consistent. Also, how can we know upfront that a stream is non-empty?

[yutakahirano]

I don't like the behavior, at least.

It makes the API consistent. Also, how can we know upfront that a stream is non-empty?

It is possible to state that .bodyState is set when locked (temporarily) or non-empty data is read from the stream (permanently).

[annevk]

I'm not sure what that means.

[yutakahirano]

A Response object has an associated used predicate that returns true if one of the following holds:

Any non-empty data has been read from the associated readable stream.
The associated readable stream is locked to a reader.

(Example)
var res1 = new Response();
assert_false(res1.bodyUsed); // body is not used.
res1.text().then((text) =>
  assert_equals(text, '');
  assert_false(res1.bodyUsed);
);

var pipe2 = ...;
// Assume we can create a response from a stream.
var res2 = new Response(pipe2.readable);
assert_false(res2.bodyUsed); // body is not used.
res2.text().then((text) =>
  assert_equals(text, '');
  assert_false(res2.bodyUsed); // We didn't modify the body.
);
assert_true(res2.bodyUsed); // body is locked.
pipe2.writable.close();

var pipe3 = ...;
var res3 = new Response(pipe3.readable);
assert_false(res3.bodyUsed); // body is not used.
res3.text().then((text) =>
  assert_equals(text, 'hello');
  assert_true(res3.bodyUsed); // We read data from the body.
);
assert_true(res3.bodyUsed); // body is locked.
pipe3.write('hello');
pipe3.close();

[yutakahirano]

It makes the API consistent.

Additionally, it is inconsistent with Request construction.

var req = new Request('http://www.example.com/');
assert_false(req.bodyUsed);
// This doesn't set |req.bodyUsed| because req's body is null.
var req2 = new Request(req);
assert_false(req.bodyUsed);
req.text().then(function() {
  // It's strange that calling text() sets bodyUsed.
  assert_true(req.bodyUsed);
});

[annevk]

Fair, in the case of a null body (rather than empty body) we should maybe make text() et al always succeed.

[wanderview]

(Sorry for my slow response. I'm on PTO.)

Happy to see .text() on null body always succeed and not set .bodyUsed. That's a special case that we already exempt in the constructor (as you noted).

So your opinion is that .bodyUsed should return if (part of) data is consumed by someone, right?

How can .text() be considered only reading part of the data? By definition .text() reads the entire body from the Request/Response. Changing it to mean anything else is not backward compatible and will break existing code.

I personally prefer setting .bodyUsed as soon as the first byte is read via .body.getReader() as well, but I'm willing to bend on that one since you guys don't agree. I think its creating a footgun that will result in partial values stored in Cache API, etc, but I won't argue it further.

It is true that fetch-with-streams draft changes the behavior.

In general I can't see any reason why adding a stream API requires breaking backward compat on the existing API.

And I have to say I'm incredibly nervous now that you are shipping this API on a release channel in a number of weeks. This breaking change was not mentioned in your "compatibility risks" in your intent to ship:

https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/fetch$20stream/blink-dev/35_QSL1ABTY/LMKZ9nTOaf8J

Are there other non-backward compatible changes to the Fetch API in the next release? Based on my review in #25 I think the answer is "no", but it would be nice to have confirmation.

Anyway, sorry to be all worked up over this.

[wanderview]

Also, I just want to say I'm optimistic we can sort this out. I'm sorry if my last comment came across a bit negative. I know its hard to do this kind of spec work and I really appreciate you guys taking it on.

[yutakahirano]

So your opinion is that .bodyUsed should return if (part of) data is consumed by someone, right?

How can .text() be considered only reading part of the data? By definition .text() reads the entire body from the Request/Response. Changing it to mean anything else is not backward compatible and will break existing code.

May be, or may not be. Without the streaming API, there is no way to read data partially, so calling text() consumes all data. With the streaming API, I think either are possible - consuming all data, or consuming remaining data. Note that I'm not opposing to the former option.

I personally prefer setting .bodyUsed as soon as the first byte is read via .body.getReader() as well, but I'm willing to bend on that one since you guys don't agree. I think its creating a footgun that will result in partial values stored in Cache API, etc, but I won't argue it further.

I think you are fine with #37 (comment), right?

[yutakahirano]

I would like to set .bodyUsed when locked, respecting Anne's idea.

When we adopt the idea that .bodyUsed is set when non-empty data is read (or locked), It looks reasonable to me that reading from an empty response doesn't set .bodyUsed.

[tyoshino]

Let's avoid confusion around "set .bodyUsed". E.g. include "permanently" if it means making .bodyUsed return true forever, and include "while" if it means making it return true only while some condition is met (for example, force .bodyUsed return true while the stream is locked).

[yutakahirano]

@tyoshino Thanks, here I rephrase #37 (comment).

I would like to set .bodyUsed at least temporarily when locked, respecting Anne's idea.

When we adopt the idea that .bodyUsed is set permanently when non-empty data is read and set temporarily when locked, It looks reasonable to me that reading from an empty response doesn't set .bodyUsed.

[tyoshino]

@wanderview Regarding

I personally prefer setting .bodyUsed as soon as the first byte is read via .body.getReader() as well,

@domenic said that a stream that has been partially (not fully) consumed using the Streams API should still be able to be text()-ed in w3c/ServiceWorker#452 (comment). You should convince Domenic.

[tyoshino]

IIRC, we were trying to re-define text(), json(), etc. using the ReadableStream when we were introducing the ReadableStream to Request/Response, while we treated the other operations such as cache.put() as special and didn't try to re-define them using the ReadableStream. We paid attention to only the state of the ReadableStream to define reasonable precondition to text(), json(), etc., and reasoned that a Request/Response with an empty stream (was originally non-empty but made to be empty by text(), json(), etc.) should be able to accept text(), json(), etc. as well as we want to allow text(), json(), etc. on a Request/Response constructed and initialized to be empty. See w3c/ServiceWorker#452 (comment)

[wanderview]

May be, or may not be. Without the streaming API, there is no way to read data partially, so calling text() consumes all data. With the streaming API, I think either are possible - consuming all data, or consuming remaining data. Note that I'm not opposing to the former option.

"Remaining data" is fine with me. It still implies EOF and bodyUsed should be true after .text() resolves, though.

I think you are fine with #37 (comment), right?

Its unclear to me if pipe2 is correct. The body is not technically null there. Its an empty stream that should hit EOF immediately. In that case it seems to me that bodyUsed should be true.

Let's avoid confusion around "set .bodyUsed". E.g. include "permanently" if it means making .bodyUsed return true forever, and include "while" if it means making it return true only while some condition is met (for example, force .bodyUsed return true while the stream is locked).

I think bodyUsed should never revert from true back to false. IMO it should mean EOF.

@domenic said that a stream that has been partially (not fully) consumed using the Streams API should still be able to be text()-ed in w3c/ServiceWorker#452 (comment). You should convince Domenic.

Yea. I gave up on that one.

[yutakahirano]

I will reply to other points later, but I would point a couple of things:

I think bodyUsed should never revert from true back to false. IMO it should mean EOF.

This contradicts with the idea I mentioned at #37 (comment).

var res = new Response('hello');
assert_false(res.bodyUsed); // body is not used.
var reader = res.body.getReader();
// (A) Should res.bodyUsed be true here?
// We didn't read anything!
reader.releaseLock();
// (B) Should res.bodyUsed be true here?

@wanderview , Let me confirm again: You prefer false for both (A) and (B)? If so, a developer should check if the body is locked in addition to check bodyUsed by adding a try-catch and that bodyUsed looks useless by itself.

A possible fix is to define bodyLocked separately.

Regarding differentiating empty and null body, it's troublesome because streams doesn't have such distinction.

// This is an empty response!
var res = new Response('');
// Here res.body is already CLOSED.
assert_false(res.bodyUsed); // body is not used.
// Note that acquiring reader actually does nothing in the current stream spec.
var reader = res.body.getReader();
// (C) Should res.bodyUsed be true here?
reader.releaseLock();
// (D) Should res.bodyUsed be true here?

reader = res.body.getReader();
// (E) Should res.bodyUsed be true here?
// |reader| is already released (because |res.body| is closed) and one cannot notify
// anything with a released reader to an underlying source.
reader.read().then(r => {
  // Nothing is actually read
  assert_true(r.done);
  // (F) Should res.bodyUsed be true here?
});

[wanderview]

var res = new Response('hello');
assert_false(res.bodyUsed); // body is not used.
var reader = res.body.getReader();
// (A) Should res.bodyUsed be true here?
// We didn't read anything!
reader.releaseLock();
// (B) Should res.bodyUsed be true here?

@wanderview , Let me confirm again: You prefer false for both (A) and (B)? If so, a developer should check if the body is locked in addition to check bodyUsed by adding a try-catch and that bodyUsed looks useless by itself.

Ok, I see what you mean here.

I think this shows the awkwardness of trying to equate the stream lock to the bodyUsed attribute, though. I find it terribly confusing to use an attribute that says "the body has been drained and is used" to mean "the body is temporarily unavailable".

Can we make bodyUsed mean stream EOF and just make .text()/etc reject if the body is locked?

It would be nice if the ReadableStream had a .locked attribute to expose this state without having to use a try/catch. Then code could check response.body.locked to detect this temporary unavailable condition.

[wanderview]

A possible fix is to define bodyLocked separately.

Oh, I missed this sentence. Yes, I think that would make sense. I think it would be nicer to make it body.locked as an attribute on ReadableStream, though.

Regarding differentiating empty and null body, it's troublesome because streams doesn't have such distinction.

I guess we can have an internal flag for null body. If there is no body at all provided in the Constructor then its set to true. If a stream is provided, but hits EOF without reading any actual data then its set to true. Then bodyUsed becomes !nullBody && streamEOF.

Is there a clean way for Request/Response to detect if any data is actually consumed from the stream?

[domenic]

It would be nice if the ReadableStream had a .locked attribute to expose this state without having to use a try/catch.

This seems fine to me; not a problem to add.

I guess we can have an internal flag for null body.

I think it would be better to get rid of this distinction between null and empty body ... I do not understand what it adds.

[wanderview]

I think it would be better to get rid of this distinction between null and empty body ... I do not understand what it adds.

I think we need some kind of internal concept for it to prevent things like Cache trying to get the stream when its already EOF'd with no-data-read. Cache and other APIS need something to check to short-circuit on.

[yutakahirano]

OK, let's provide bodyLocked on Response or Readable[Byte]Stream. I'm not sure which is better.

I think we need some kind of internal concept for it to prevent things like Cache trying to get the stream when its already EOF'd with no-data-read. Cache and other APIS need something to check to short-circuit on.

From Streams point of view, one can do nothing with a CLOSED stream. It cannot be locked, non-empty data cannot be read from it and it cannot be errored. Hence I cannot find any point at which we can set bodyUsed. In the latter example in #37 (comment), bodyUsed was not set initially. In order to see true at (F), we should choose when to set it from (C), (D), (E) and (F). None of them looks reasonable to me.

As a fetch-streams user, I still don't understand why null vs. empty distinction is important in this use-case (I'm not saying that the distinction is generally useless).

[wanderview]

OK, let's provide bodyLocked on Response or Readable[Byte]Stream. I'm not sure which is better.

I would really prefer Response.body.locked. I don't see why detecting the lock state is a fetch-specific thing. Every stream instance would benefit from this.

From Streams point of view, one can do nothing with a CLOSED stream. It cannot be locked, non-empty data cannot be read from it and it cannot be errored. Hence I cannot find any point at which we can set bodyUsed. In the latter example in #37 (comment), bodyUsed was not set initially. In order to see true at (F), we should choose when to set it from (C), (D), (E) and (F). None of them looks reasonable to me.

I'm not asking to set bodyUsed to true at (F).

I'm saying the Cache API needs some way to avoid an error when doing cache.put() with a closed stream, but where no data was read and bodyUsed is false. In this case Cache API cannot just try to read from the stream because it will error on either the lock or closed state. Cache API needs some flag to detect that we're doing the don't-error-on-empty-stream optimization.

[domenic]

I'm saying the Cache API needs some way to avoid an error when doing cache.put() with a closed stream, but where no data was read and bodyUsed is false.

Why?

In this case Cache API cannot just try to read from the stream because it will error on either the lock or closed state.

If it is locked it will error (and that seems like a good thing). If it is closed it will just get { done: true, value: undefined }.

[wanderview]

If it is locked it will error (and that seems like a good thing). If it is closed it will just get { done: true, value: undefined }.

I was thinking multiple reads on closed stream would reject.

I guess it will just work if:

1. Response/Request don't set bodyUsed when the stream hits EOF without any data being read.
2. .text(), cache API, etc unlock the stream after they get the done:true signal.

So you're proposing just having the Request and Response constructors create an empty body stream in the case of no body being provided?

[domenic]

2. .text(), cache API, etc unlock the stream after they get the done:true signal.

This happens automatically. As @yutakahirano says once a stream has been read-to-end (aka closed) it is automatically unlocked.

So you're proposing just having the Request and Response constructors create an empty body stream in the case of no body being provided?

I think so. I'm still a little confused at what the goal of bodyUsed is these days but mainly just trying to chime in to make sure the model stays coherent. This null vs. empty body things feels incoherent.

[wanderview]

I'm still a little confused at what the goal of bodyUsed is these days but mainly just trying to chime in to make sure the model stays coherent.

Well, go look at how "body used" is referenced in these specs:

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
https://fetch.spec.whatwg.org/#concept-body-used-flag

Its essentially a safety-belt to prevent script from storing a drained Response in Cache or uploading a drained Request to a fetch PUT, etc.

The previously proposed changes of unsetting bodyUsed when the stream lock was dropped broke all that. It would have also broke client code checking bodyUsed.

Maintaining the definition of bodyUsed as "body EOF and body was not an empty stream" keeps all that working.

If the Request/Response can detect the "body was an empty stream" condition and set bodyUsed appropriately, then I'm ok with always setting a body stream. Using null was just a convenient way of doing that previously.

[wanderview]

Perhaps once you invoke getReader() the used flag gets set?

Yes, that's what I'm proposing in #43.

That works for me, but I recall there were objections when I asked for "immediately used" in the past.

[domenic]

@yutakahirano wow, OK, that is a very interesting problem.

I guess I have to ask: what if we just never set Content-Length in fetch? (I.e., always used chunked encoding.) I have heard Content-Length is untrustworthy and stuff.

But, I anticipate this not being what people want. I just had to ask.

I would like to disallow fetch(req2) by #43 or modifying streams. (Note: I'm not against whatwg/fetch#61, but used flag on Request / Response only is not enough).

See #43 (comment).

---

I really didn't consider Content-Length when designing streams. But in retrospect it's obviously important. You can imagine similar situations where you're dealing with a (total length, stream) pair. In those cases the offset into the stream is definitely important information. (This is also another I/O streams vs. abstract async sequences thing IMO.)

bytesReadSoFar is starting to sound reasonable. (Or use the size() function to make it something that works for all readable streams.) I realize this is a complete 180, but this use case really surprised me.

If people have other convincing arguments or scenarios where bytesReadSoFar is important---preferably not for bodyUsed-style warnings, but instead of stuff like Content-Length---it would definitely help tip me over the edge.

[annevk]

My thinking was that if you upload using a stream, you'd always get chunked. Fetch needs a bunch of tweaks for that, but should be doable.

[domenic]

Sure, but the point of #37 (comment) is that you're always "uploading using a stream", even if you pass in e.g. a string as the request body.

[annevk]

Yeah, so my suggestion for that case is that you can't do that. If you're using the Request object, you should have cloned it before passing it to fetch().

As to your question about Content-Length. Currently all browsers use Content-Length exclusively for <form> and XMLHttpRequest and such. fetch() should be able to do it.

[tyoshino]

#37 (comment)

(Or use the size() function

Did short analysis on size() approach.

What kind of size() did you think about? One returning a number only when the underlying source has close()-d? (One returning current size regardless of source-side-closure doesn't help since the size may still grow.)

• This size() which work only when the stream is "finalized" works for addressing the Content-Length issue.
• It cannot be used for detecting partial drain as the stream may initially be non-finalized and still be non-finalized when fetch(), cache.put() is called. I.e. there's no way to see diff of size().
• It cannot be used for detecting full drain if the stream is initially non-finalized. Even if it's fully drained, i.e. size() returning a number and the number is 0, it's possible that nothing has been enqueued to the stream but just finalized after we check size() when constructing the Request/Response.

[tyoshino]

Will the bytesReadSoFar be un-resettable for its lifetime given wanderview's requirement (#37 (comment))? If so, it is not good from my point of view ((3) of #37 (comment)).

[tyoshino]

Anne, #37 (comment)

Yeah, so my suggestion for that case is that you can't do that.

Does "that" here mean reading some part of the fixed string from .body and then pass the request containing the rest of the string to fetch()?

[annevk]

Yes. Basically, once you use a request/response's stream for A, you can no longer use it for B.

[domenic]

@annevk

Yeah, so my suggestion for that case is that you can't do that. If you're using the Request object, you should have cloned it before passing it to fetch().

I guess that is OK. At least it is simple. We are basically saying "don't touch .body for Requests" on the client (uploader) side, but only on the server (service worker) side. And similarly don't touch .body for Responses on the server side, but only on the client (downloader) side. And the FetchReadableByteStream type from #43 (comment) is what enforces this.

@tyoshino

What kind of size() did you think about?

Sorry, I simply meant using the queuing strategy size() method to calculate "sizeSoFar" or something, instead of making it byte-specific. So every time something is read (dequeued) we not only change the value of controller.desiredSize but also of rs.sizeSoFar. (sizeReadSoFar??) Then:

• The Content-Length issue is addressed by calculating new Content-Length = original Content-Length minus sizeSoFar.
• We can detect partial drain or full drain if sizeSoFar is > 0.

I did not anticipate it being resettable. I am not sure how that impacts (3) of
#37 (comment) though. We wouldn't need a wrapper if we added sizeSoFar, I think.

The motivation for this sizeSoFar is basically the idea that streams with a total length are probably a common pattern. Maybe HTTP is a special case though---e.g. file streams often do not have total sizes, since you just open the stream instead of stat'ing first. People seem less excited about this idea than I initially was.

[tyoshino]

Added a new item (7) based on the concern yutakahirano has been discussing to #37 (comment)

[tyoshino]

Yet another alternative "ReadableByteStream observing API" posted at whatwg/streams#362

[tyoshino]

@domenic

Sorry, I simply meant using the queuing strategy size() method to calculate "sizeReadSoFar" or something, ...

I see, sorry!

The Content-Length issue is addressed by calculating new Content-Length = original Content-Length minus sizeReadSoFar.

OK. So, we store the Content-Length separate from the stream.

---

At which point should we guarantee sizeReadSoFar gets updated? Valid while not locked?

[wanderview]

Another thought I had about marking "used at end of stream" instead of "used on first byte read":

In many cases a Response is effectively read-only. It does not allow modifications. However, letting code read a portion of the stream and then storing it in Cache or passing it on effectively is a modification. Its a deletion of data from the body.

Is that ok in these explicitly read-only cases? @annevk?

[annevk]

It's not clear to me what that buys us. Seems more confusing.

[tyoshino]

Wrote a sketch of Domenic's proposal of reader.bytesRead at whatwg/streams#367.

[tyoshino]

Please take a look at analysis of requirements for the bytesRead approach at whatwg/streams#367 (comment). Are you guys fine with dropping (5) and (7)?

[yutakahirano]

I'm fine with that.

[annevk]

I'm not convinced this can be used for Content-Length. As per https://lists.w3.org/Archives/Public/public-webapps/2014OctDec/0422.html and other emails in that thread. Pretty sure that whenever developer-controlled streams are involved we need to use chunked encoding.

[domenic]

This is not necessarily about developer-controlled streams. This is about e.g. getting a Request from the cache, reading a few bytes from it, then trying to upload (which I believe is supposed to be disallowed).

[wanderview]

This is not necessarily about developer-controlled streams. This is about e.g. getting a Request from the cache, reading a few bytes from it, then trying to upload (which I believe is supposed to be disallowed).

Right. I think we should disallow that.

I think @annevk is suggesting we should set the used flag immediately on getReader(). It sounds like you still want to set used based on offset somehow. Is that correct?

[tyoshino]

@annevk Wrote a concrete algorithm at whatwg/streams#367 (comment). This doesn't change anything for the problem how to generate the Content-Length header for developer-controlled streams, but addresses the "drained" detection as Domenic just said.

[yutakahirano]

Another point: We need to track cancel called by the reader-side. For example,

var req = new Request(url, {body: 'hello'});
req.body.getReader().cancel();
var req2 = new Request(req);  // should throw

#43 has no problem, because it sets used flag when locked.

[yutakahirano]

We will use IsDisturbed(stream) to define bodyUsed. See whatwg/streams#378 and https://etherpad.mozilla.org/streams-f2f-july.

[tyoshino]

Finally, this came to an agreement and fix has been made on the spec!

whatwg/streams#385 (comment)