`readableStreamFromIterable` kills the process

[KnorpelSenf]

Description

readableStreamFromIterable from io can be used to convert async generators into streams in order to send them via fetch. However, if the generator throws, this leads to an uncaught promise rejection, hence killing the Deno process.

While I believe this problem to be fixable by readableStreamFromIterable, it might rather be a bug in the underlying ReadableStream defined in deno/ext/web/06_streams.js.

Reproduction steps

Create a file evil.ts with the code

import { readableStreamFromIterable } from 'https://deno.land/std@0.106.0/io/mod.ts'
async function* evil() {
    throw new Error('Catch me if you can')
}
const itr = evil()
const stream = readableStreamFromIterable(itr)
try {
    await fetch('https://deno.land/', { method: 'POST', body: stream })
    console.log('Sent.')
} catch (e) {
    console.error('Sending failed!', e)
}

and run it using

deno run --allow-net evil.ts

Note that this will not actually perform any network request.

Expected behaviour

The request should fail, and the thrown error should be logged to the console, preceded by 'Sending failed! '.

Actual behaviour

The process is killed.

error: Uncaught (in promise) Error: Catch me if you can
    throw new Error('Catch me if you can')
          ^

Desktop

• OS: Debian 10
• Version: 4.19.0-17-amd64

deno 1.13.2 (release, x86_64-unknown-linux-gnu)
v8 9.3.345.11
typescript 4.3.5

[crowlKats]

This is indeed coming from actual ReadableStream implementation, but this behaviour is correct

[nayeemrmn]

I ran this in Chrome and Firefox, the error is also uncatchable i.e. it's not propagated to the fetch() call or anything. So behaving as expected.

[KnorpelSenf]

I see. Is there any way how I can catch the error, or do I just have to tell users that they must not ever pass iterators that could throw?

[nayeemrmn]

It doesn't seem like there's a way to catch it. But if you have an API which accepts iterators and passes them to readableStreamFromIterable(), you can wrap it.

import { readableStreamFromIterable } from "https://deno.land/std@0.106.0/io/mod.ts";

export async function myLibFunction(iterable: AsyncIterable<Uint8Array>) {
  // Wrap the iterable.
  const safeIterable = (async function* () {
    try {
      yield* iterable;
    } catch (e) {
      console.error("Sending failed!", e);
    }
  })();
  // Fetch with the safe iterable.
  const stream = readableStreamFromIterable(safeIterable);
  try {
    await fetch("https://deno.land/", { method: "POST", body: stream });
  } catch (e) {
    console.error("Sending failed!", e);
  }
}

// User code:
async function* evil() {
  throw new Error("Catch me if you can");
}

await myLibFunction(evil());

What's your exact use case? We may be able to make this cleaner.

[KnorpelSenf]

That's what I've been thinking about, however, I would like to reject in the API method.

The API exposed to my users should take an iterator of Uint8Array objects, and perform a fetch call with them. It already wraps the iterator a few times for several enhancements that are irrelevant here.

Eventually I'd like to achieve something like this to be possible:

try {
  await apiMethod(evilIterator)
} catch (e) {
  console.log('I passed an evil iterator', e)
}

The only way how I see this would be possible is to start working based on your suggestion, and then drag along an AbortSignal and a reject function.

Once the iterator rejects, we cancel the request, and in turn call reject.

I'm relatively sure that this is possible, and I knew how I'd implement it—it's just a relatively verbose workaround given that readable streams could also just reject.

I don't remember the part of the web spec that justifies unhandled rejections, but it would just seem pretty natural to me that the default reader rejects: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/read#return_value

[nayeemrmn]

Okay you'll have to do this:

import { deferred } from "https://deno.land/std@0.106.0/async/mod.ts";
import { readableStreamFromIterable } from "https://deno.land/std@0.106.0/io/mod.ts";

export async function myLibFunction(iterable: AsyncIterable<Uint8Array>) {
  // Wrap the iterable.
  const iterableResult = deferred();
  iterableResult.catch(() => {});
  const safeIterable = (async function* () {
    try {
      yield* iterable;
      iterableResult.resolve();
    } catch (error) {
      iterableResult.reject(error);
    }
  })();
  // Fetch with the safe iterable.
  const stream = readableStreamFromIterable(safeIterable);
  const abortController = new AbortController();
  await fetch(
    "https://deno.land/",
    { method: "POST", body: stream, signal: abortController.signal },
  );
  try {
    await iterableResult;
  } finally {
    abortController.abort(); // EDIT: Actually I think this is a redundant time to abort, but you can't abort a locked stream right now...
  }
}

// User code:
async function* evil() {
  throw new Error("Catch me if you can");
}

await myLibFunction(evil());

Though it sounds like you know what you're doing :-)

I don't remember the part of the web spec that justifies unhandled rejections, but it would just seem pretty natural to me that the default reader rejects: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/read#return_value

You're right, pull() errors do cause rejections in ReadableStreamDefaultReader::read(). However, internal ReadableStreamDefaultReader::read() rejections do not cause rejections in fetch(). They are just detached async errors in Deno as well as browsers. So they need to be intercepted earlier.

[KnorpelSenf]

Thanks for implementing this for me! 😊 That saves me some time with this.

deferred is cool, I didn't know that yet! Unfortunately, Deno is just one of the runtimes I'm targeting, so any Deno-specifics need to replicated across other platforms. I'll therefore adjust the example a bit.

To complicate things a little further, users are given the ability to pass a custom AbortSignal to myLibFunction, just like to fetch. But I guess we can just listen to the passed signal and “forward” the event to our own AbortController.

I can drop a link to the final implementation here for anyone who's interested.

@nayeemrmn thanks so much for your help! 🎉

[KnorpelSenf]

Working on this in https://github.com/grammyjs/grammY/pull/78