Improve fetch()

[ry]

Depends on fetch landing (#512) which will happen in v0.1

For v0.2:

1. support streaming bodies
2. support binary data from fetch:

   deno/js/fetch.ts

   Line 49 in 168d92f

   throw Error("not implemented");

[benjamingr]

Does body.getReader() return a ReadableStreamDefaultReader or a deno Reader - I'd expect the latter.

[ry]

@benjamingr It should follow the DOM semantics - we'll have a helper to turn it into a deno reader.

[benjamingr]

@ry that effectively means shipping ReadableStream and ReadableStreamDefaultReader though and two stream abstractions out of the box.

[ry]

@benjamingr True - but I think we should hold to the promise of DOM compatibility.

[ry]

Fetch has been improved a lot since the creation of this issue - the major missing piece is now streaming response body. I will probably work on this soon - but in case someone is interested - here's a bit of an outline of how it should be done.

• Fetch response bodies should be registered as resources in src/resources.rs so that we can use the zero-copy deno.read() op with them. See

  deno/src/resources.rs

  Lines 43 to 48 in 3f1899f

  	enum Repr {
  	Stdin(tokio::io::Stdin),
  	Stdout(tokio::io::Stdout),
  	Stderr(tokio::io::Stderr),
  	FsFile(tokio::fs::File),
  	}

• Probably this means storing a hyper::Body in the resource table.
• On the JS side Response.body should yield a DOM compatible ReadableStream but should also mixin a Deno specific read() method such that it implements the deno.Reader interface. (Although we should support ReadableStream to be web-compatible -- I consider it a poorly designed API -- Deno uses a better Go-inspired interface.)

If you're interested in working on this, be sure to sync with me first over email or gitter.

[benjamingr]

@ry talking to Jake and the fetch authors - it is probably fine (to a reasonable degree) to support compatibility through async iterators (since both Deno and ReadableStreams are becoming AsyncIterable). Although that's a concern to consider.

[jeromegn]

We've implemented streaming bodies in superfly/fly.rs and it makes a pretty significant difference in performance/memory usage.

We have a "body mixin" which both Response and Request extend.

We use the very good (and very new), typed, @stardazed/streams package for our ReadableStream implementation.

I'm happy to port some of these changes over and adapt them.

One thing I'm not so sure about is we added a concept of cmd_id: 0 for unprompted "events" coming from rust. If the cmd_id === 0, then we pass the message to a secondary handler which will dispatch to either an event listener or to a "chunk enqueuer" (which is just a ReadableStream.)

Another difference, not sure if it matters, is: we don't keep track of "data" passed raw into v8. We assume it's consumed right away and GC the buffer in rust with a callback.

[ry]

Just a note that streaming response bodies should be registered in the resources table - so they can use the read() and write() ops and interop with other “streams” (aka Reader / Writer)
https://github.com/denoland/deno/blob/master/src/resources.rs

[jeromegn]

We could add them in there for sure, but ReadableStreams are meant to be "pushed into" (I think). Reading from a stream from v8 makes sense with the current model, that's for sure, but it also incurs a cost.

Polling to read requires 1 extra message per chunk (ask to read, get a read chunk), while pushing is just 1 message (the read chunk). Normally I don't think it would matter that much, but passing more messages for such frequent messages might be too heavy.

For things like stdin, it doesn't make much sense to dump everything in V8 if it's never going to be read. It would just accumulate in-memory and never be "done". I think most other readable IOs could be pushed and use ReadableStream?

We also push from V8 to write back http response bodies.

Once a stream is "done", we immediately delete it from the JS Map, so memory will be freed pretty quickly by V8's GC if nothing is holding onto it outside the map. If the user holds onto the response or the request, they'll be able to still read from the body.

There's a fair chance I don't know enough, but that technique seems to work for us at the moment.

[ry]

@jeromegn

1. The overhead of going back to V8 for each packet is something we are going to optimize to death.
2. https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/getReader is a pull API.
3. For unbounded streams like this, push events are bad because there is no back-pressure. Lack of back-pressure ultimately results in high jitter - bad tail latency. This was another major design mistake of Node.

[benjamingr]

For unbounded streams like this, push events are bad because there is no back-pressure. Lack of back-pressure ultimately results in high jitter - bad tail latency. This was another major design mistake of Node.

Node streams have a pull mode though for ever now which is what's typically used in real applications. That said Node streams are a mess and pretty much no one likes them :)