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

Does FetchBodyStream need to be split #413

Closed
annevk opened this issue Aug 13, 2014 · 22 comments
Closed

Does FetchBodyStream need to be split #413

annevk opened this issue Aug 13, 2014 · 22 comments

Comments

@annevk
Copy link
Member

annevk commented Aug 13, 2014

Per @domenic we might need both a read and write a stream rather than just one. That would change how Request/Response work pretty drastically.

@jakearchibald
Copy link
Contributor

What are the use-cases? Is this to explain something like EventSource?

@annevk
Copy link
Member Author

annevk commented Aug 13, 2014

The problem is that the IO streams specification only acknowledges input and output streams. Not a concept that is both simultaneously, which is more or less what we need. So we need to make this an object that exposes both or some such.

@domenic
Copy link
Contributor

domenic commented Aug 13, 2014

So the issue is that there is no such thing as a stream that is both readable and writable. That concept is represented by a pair { input, output } of readable and writable streams. (Side note: it is confusing which one of those two is which -_-) Even if we were to move to a design where you did not create such streams separately with their own constructors, but instead always created a linked pair and then only vended the side you want to expose, we would still have that "linked pair" design instead of a "smushed into one object" design.

Last time we discussed this my conclusion was that doing things "the Node way," where you have a ClientRequest that is writable (for performing uploads) and a ServerRequest that is readable (for receiving uploads) was too painful for the common use cases we were seeing in service worker and related code, e.g. wanting to get an incoming "server" request and then pass it to fetch as a "client" request. The code would end up looking something like this, which is similar to the kind of code you write in Node, and just as painful.

That leaves us wanting to combine both concepts into a single Request class, which contains a pass-through stream (i.e. a readable + writable pair where the write side feeds directly into the read side; also known as, an identity transform stream). For client requests, the developer will write to the writable stream and the browser will read from the readable side. For server requests, the developer will read from the readable side as the browser writes into the writable side.

API-wise, the impact is that, when we expose the underlying stream machinery, you'd probably do things like

// I'm on the server!
self.onfetch = ({ request: req }) => {
  req.body.input.pipeTo(aWritableDestination);
  req.body.input.read(); // reads a chunk if available
  req.body.input.readInto(arrayBuffer, 0, 1024); // reads up to 1024 bytes into arrayBuffer starting at position 0
};
// I'm on the client!
var req = new Request("http://example.com", {
  method: "POST",
  headers: { "Content-Length": 1024 }
});

fetch(req).then(...);

req.body.output.write(new ArrayBuffer(512));
aReadableSource.pipeTo(req.body.output);

(Here I am using the version from the above-linked bug where output is writable and input is readable, instead of the version currently in the streams spec which is the other way around -_-)

Considerations impacting MVP:

  • Where should we put asJSON() and friends? This ties into the ongoing discussion about what those methods mean semantically. On req.body seems fine. On req.body.input seems fine as long as they consume the stream, as then they are part of the low-level stream interface. Even on req seems OK.
  • If we want to put it on req.body.input, then they need to move, and we need to be sure input is the right name. Frankly, the input and output naming seems worse all the time, especially given my confusion linked above. Suggestions welcome. Ideally suggestions should also work for non-identity transform streams.

@jakearchibald
Copy link
Contributor

Isn't it just:

// I'm on the client!
var requestBody = new ReadableStream(function() {
  start: function(enqueue, close, error) {
    enqueue(new ArrayBuffer(512));
  }
});

var req = new Request("http://example.com", {
  method: "POST",
  headers: { "Content-Length": 1024 },
  body: requestBody
});

// or

var req = new Request("http://example.com", {
  method: "POST",
  headers: { "Content-Length": 1024 },
  body: aReadableSource
});

@domenic
Copy link
Contributor

domenic commented Aug 13, 2014

That adds layers of indirection between the actual writable socket representing the outgoing HTTP request, and the user. The goal of the low-level stream API is to expose that socket, along with its state, backpressure signals, kernel-level buffering, etc. as something you can write to. If you have a source of data you want to write to that socket, the way to express that is by piping the readable stream representing that source to the writable stream representing that sink: i.e., there is already an idiomatic way to express such an operation, that makes it clear what is going on, instead of making it implicit that when you pass a readable stream to the Request constructor, it will consume it implicitly.

@jakearchibald
Copy link
Contributor

After some chatting on IRC, I think we're leaning towards a function that would reveal a writable stream for request/response.

new Request(url, {
  method: 'POST',
  streamer: function(writableStream) {
    // ...
  }
});

If you had a readable stream & wanted to use that as a request/response body, you can still do:

new Request(url, {
  method: 'POST',
  body: readableStream
});

The issues are:

  • streamer is a terrible name
  • streamer and body clash - what if both are set?

We could continue to overload body, which can already take a string, blob, arraybuffer, formdata, stream, and allow it to take a function too which would reveal the writable stream. That kinda feels like an overload too far, but it deals with both the streamer naming and body clash.

@domenic is that a fair roundup?

@tyoshino
Copy link
Contributor

I like this idea.

For example, thinking of whatwg/streams#97, I think we basically want the streamer way to return a WritableStream using which we can write contents directly to the Response object than requiring a joint (generic identity transform stream) between them where no transform is needed.

terrible name

{reveal|expose|create}Body{Stream|StreamWriter|WritableStream}?

both are set

Maybe initialize the stream with the data provided by the RequestInit.body? Just throwing a TypeError also sounds fine to me.

Add function to the overload

Hmm, are we sure that there won't be any more type in the future that also needs the revealing pattern?

@domenic
Copy link
Contributor

domenic commented Aug 18, 2014

@jakearchibald ya that's a fair roundup indeed.

terrible name

I'm leaning toward something like {start|init|with}BodyStream?

both are set

TypeError for now seems best; we could relax it later if there's a compelling use-case, but I kind of doubt there would be.

Add function to the overload

I was only tempted to do this when I couldn't think of a name, but @tyoshino got my thoughts flowing again in that regard and so I don't think it's worth it.

@willchan
Copy link
Collaborator

Dumb lurker questions:

  • Why do we need to expose a WritableStream for a response?

If you have a source of data you want to write to that socket, the way to express that is by piping the readable stream representing that source to the writable stream representing that sink

  • I am probably missing the context, but I don't completely follow here. I agree with the statement, but I don't see why that means we need a WritableStream for the body. If you passed in a ReadableStream as body, wouldn't the UA implementation of Request naturally pipe that ReadableStream directly out to the socket?

@annevk
Copy link
Member Author

annevk commented Aug 19, 2014

Why do we need to expose a WritableStream for a response?

So you can pipe data to a document from a service worker.

@willchan
Copy link
Collaborator

So you can pipe data to a document from a service worker.

I read and re-read this a few times, and I think I understand the implication here. Now that I think about it, it kinda makes sense. I believe the implication is you have a Request/Response pair from the Document, and the SW has a separate Request/Response pair from a fetch() (or a Cache or what not), and you want to pipe from the SW fetch()'s Response to the Document's Response.

Did I get that right? Is that the implication? If so, that's fine, although I have to say, it's initially confusing that a Response would be writable. But if you reuse the same interface/object, for consuming as well as producing Responses, then I guess that makes sense. I kinda like how the Streams approach explicitly separated the producer/consumer into different interfaces (writable vs readable streams), but I don't know anything about web interfaces so I'll shut up now. Thanks for the explanation.

@annevk
Copy link
Member Author

annevk commented Aug 19, 2014

The document creates a Request, say for <img> and then fetches it. The service worker decides to generate a custom Response and pass that back to the document. With a writable stream it can push bytes to <img> incrementally.

@jakearchibald
Copy link
Contributor

@willchan in terms of the API, you'll only get the writable stream via function passed to the constructor

@willchan
Copy link
Collaborator

@jakearchibald Ah, perfect! It all makes sense now :)

@tyoshino
Copy link
Contributor

@domenic

I'm leaning toward something like {start|init|with}BodyStream?

As Request.body is FetchBodyStream which contains Stream in it, I suggest that we include some word in addition to Stream which implies that we're doing something further. The name should also be good enough to be distinguishable from RequestInit.body. RequestInit.body also inits FetchBodyStream (its type is named FetchBodyInit).

{init|with}Body{Writable|Writing}Stream
{init|with}BodyWriter
{init|with}StreamToWriteBody

Too long?

@annevk
Copy link
Member Author

annevk commented Aug 19, 2014

I think the input should still be RequestInit.body. You just pass a writable stream object of sorts there, whatever we design for that.

@jakearchibald
Copy link
Contributor

You could create a null transform stream & use that as RequestInit.body. Pretty sure that was suggested in IRC. @domenic, why was that insufficient?

@domenic
Copy link
Contributor

domenic commented Aug 19, 2014

@tyoshino I see what you mean. But perhaps FetchBodyStream just needs renaming to FetchBody or similar. That said, bodyWriter(stream) { stream.write(chunk); } seems OK.

@annevk but you (the browser, I mean) can't read from a writable stream. The creator of the writable stream needs to hook things up to the underlying sink for you ahead of time. @jakearchibald's identity transform stream idea is closer: you create a paired set of readable/writable streams, and pass the readable one as body. But that's not great because it doesn't let the browser expose the underlying sink (socket) as a writable stream directly, and thus precludes out-of-main-thread piping or writing, since it has to pass through the identity transform (which can be observed).

It still seems fine to do body: sourceReadableStream as sugar for bodyWriter(dest) { sourceReadableStream.pipeTo(dest); }. But the latter allows more powerful and flexible patterns, e.g. piping two streams in sequence, or manually writing chunks of a protocol as they are computed, or similar.

@tyoshino
Copy link
Contributor

@domenic OK. If such renaming happens, yes

@KenjiBaheux
Copy link
Collaborator

I think #372 (comment) gets us out of "impacts MVP" realm.

@annevk
Copy link
Member Author

annevk commented Oct 13, 2014

Yeah, at this point streams can be sorted out behind the scenes and plugged into Response/Request when ready.

@annevk
Copy link
Member Author

annevk commented Oct 30, 2014

There's a separate discussion going on already on streams integration in #452. Closing this.

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

No branches or pull requests

6 participants