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

Response constructor with ReadableStream #63

Closed
yutakahirano opened this issue Nov 27, 2015 · 21 comments
Closed

Response constructor with ReadableStream #63

yutakahirano opened this issue Nov 27, 2015 · 21 comments

Comments

@yutakahirano
Copy link
Owner

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
Copy link
Owner Author

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

@yutakahirano
Copy link
Owner Author

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

yutakahirano added a commit that referenced this issue Dec 2, 2015
This is an initial proposal for #63.
@yutakahirano
Copy link
Owner Author

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
Copy link

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
Copy link

annevk commented Mar 24, 2016

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
Copy link

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
Copy link
Owner Author

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
Copy link

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
Copy link

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

@domenic
Copy link
Contributor

domenic commented Apr 8, 2016

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

@wanderview
Copy link

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
Copy link

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
Copy link
Contributor

domenic commented Apr 8, 2016

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
Copy link

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
Copy link

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

@wanderview
Copy link

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

evt.respondWith(response, { waitForBody: true });

@yutakahirano
Copy link
Owner Author

@horo-t for serviceworker lifetime discussion.

@yutakahirano
Copy link
Owner Author

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
Copy link

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
Copy link
Contributor

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 w3c/ServiceWorker#882.

annevk pushed a commit to whatwg/fetch that referenced this issue Apr 14, 2016
This change enables constructing a Response from ReadableStream, as discussed in yutakahirano/fetch-with-streams#63.
@yutakahirano
Copy link
Owner Author

Merged as whatwg/fetch@4924f6d.

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

No branches or pull requests

7 participants