yutakahirano / fetch-with-streams

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

Response constructor with ReadableStream #63

Closed yutakahirano closed 8 years ago

yutakahirano commented 8 years ago

Creating a Response from a stream is useful, especially in a service worker. With the feature a developer is able to stream a response with an arbitrary body to the controlled page with responseWith.

yutakahirano commented 8 years ago

This is a different issue from #44 (It is about the difference between ReadableStream and ReadableByteStream).

yutakahirano commented 8 years ago

@gwicke might be interested (sorry for the delay).

yutakahirano commented 8 years ago

Hi!

We (Chrome) are interested in providing this feature. We would like to hear from users and other UA implementers about

Chrome has an experimental interface implementing the proposed interface behind an experimental flag (though there's a difference between the proposal and the implementation right now). @jakearchibald wrote a blog entry introducing the feature and may know people interested in it.

cc: @wanderview @tyoshino @domenic @gwicke @annevk @KenjiBaheux

Thanks!

jakearchibald commented 8 years ago

The performance benefits speak for themselves. I'm aware of high-profile sites that want to use this to boost performance and provide an offline-first experience. I might be able to share these in private.

I think there are a lot of questions around how streams would work with requests (in terms of redirects, and how stream pulling may correspond with progress), but I don't think response streams are contentious.

@wanderview @hober

annevk commented 8 years ago

Yeah, I thought we already agreed to add this to Fetch. I've just been waiting for a patch. I think we should just do the request bit too, but happy to go slow.

n8schloss commented 8 years ago

I don't have much to add here, but from a developer perspective, adding the ability make a stream that we can use in a response will be pretty impactful! I'm excited for this to get into production :)

yutakahirano commented 8 years ago

Thank you for the comments.

I would be happy if some implementers could give some comments. @wanderview, @youennf, @othermaciej

We'll merge the proposal to the fetch spec and start shipping process in Chromium in Apr if no concerns are raised.

wanderview commented 8 years ago

LGTM.

I assume if a service worker provides a js-backed stream to the Response passed to respondWith(), then that code is responsible for holding the FetchEvent.waitUntil() until it writes the last byte in the stream. Is that correct?

jakearchibald commented 8 years ago

Huh, I hadn't considered that. Good point. I should fix my demos in that regard.

domenic commented 8 years ago

Disclaimer: I don't know much about service worker waitUntil/respondWith. But shouldn't it implicit wait until the stream is closed?

wanderview commented 8 years ago

Well, for a fetch() returned Response the service worker thread can exit immediately after respondWith() resolves even if the body is still streaming in over the network. The data is just copied behind the scenes. This is also true for cache supplied streams since it can pull the body off the disk in a background thread.

I think if we wanted to extend the worker lifetime for a js supplied stream we would need to do a brand check. Iff its a js defined stream then extend lifetime until stream close. Would that be acceptable?

jakearchibald commented 8 years ago

We could, but that means keeping the SW open in some cases where we don't need to, eg respondWith(fetch(event.request)), which may be a 3-hour video.

We could also try and special-case constructed streams in some way, but that feels messy.

domenic commented 8 years ago

I think if we wanted to extend the worker lifetime for a js supplied stream we would need to do a brand check. Iff its a js defined stream then extend lifetime until stream close. Would that be acceptable?

You mean a brand check to distinguish between an author-constructed ReadableStream instance and a browser-constructed ReadableStream instance? That's not exactly a "brand" check, but I see. Very interesting... it does seem weird, and I'm not even sure how you'd write spec text for it, but it probably does match author intent...

I guess I'm happy to let others weigh in on this based on their service worker experience. I'm a little worried it caught @jakearchibald by surprise though. And I'm thinking of scenarios where authors don't do waitUntil() and just get caught by surprise when their streaming data gets cut off in the middle by some browsers but not others.

wanderview commented 8 years ago

We could also try and special-case constructed streams in some way, but that feels messy.

Yes, this is what I was asking about.

I suppose the SW script could pass the reader's closed promise to waitUntil. It would have to lock the stream, pass the closed promise, then unlock the stream, then pass the stream to the Response constructor?

wanderview commented 8 years ago

Maybe we could add a convenience method that does the locking/unlocking bits? Something like evt.waitUntilClosed(aStream).

wanderview commented 8 years ago

Or an optional argument to respondWith(). Something like:

evt.respondWith(response, { waitForBody: true });
yutakahirano commented 8 years ago

@horo-t for serviceworker lifetime discussion.

yutakahirano commented 8 years ago

I suppose the SW script could pass the reader's closed promise to waitUntil. It would have to lock the stream, pass the closed promise, then unlock the stream, then pass the stream to the Response constructor?

When a reader is released its closed promise is rejected and cannot be used to monitor the stream state.

FYI: Currently Chrome lets a service worker be alive while transferring data, so

We could, but that means keeping the SW open in some cases where we don't need to, eg respondWith(fetch(event.request)), which may be a 3-hour video.

is what we are doing.

jakearchibald commented 8 years ago

F2F: we're going to continue with waitUntil, but may look at ways stream controllers could provide a closed promise to make this easier.

tyoshino commented 8 years ago

I'd like to add a little more from discussion with horo-t and yutakahirano.

I've created a new issue at the ServiceWorker repo https://github.com/slightlyoff/ServiceWorker/issues/882.

yutakahirano commented 8 years ago

Merged as https://github.com/whatwg/fetch/commit/4924f6d6a2b9cdbdadffc1885141542311bda986.