Streams aren't implemented according to Node.js documentation

[gordonmleigh]

According to the Node.js docs stream implementations should override _destroy and not destroy as you have, and they should accept a second parameter which is a callback. Without the callback, async iteration breaks.

I can contribute a PR unless there's a reason the spec hasn't been followed?

[thejoshwolfe]

oh cool. Yes, there's a reason the spec hasn't been followed: this feature was written before node v8 was released. I even wrote this post on stackoverflow complaining about the lack of a destroy api: https://stackoverflow.com/a/34548102/367916 (and then updated it just now to mention that a formal api exists now).

This library supports very old versions of node, so there would need to be some kind of conditional override. This complexity will have some rippling effects elsewhere in the API as well.

Can you give an example of the async iteration that breaks without the proper destroy api?

[gordonmleigh]

Ah right. Interesting.

Simple repro below:

// specifics of next three lines not important
const zip = await openZip('/path/to/zip'); 
const entry = await getEntry(zip); 
const stream = await openReadStream(entry);

for await (const chunk of stream) {
  throw new Error('BANG!');
}

The above for-await-of translates roughly to the following:

const iterator = stream[Symbol.asyncIterator]();
let item;

try {
  for (; item = await iterator.next(), !item.done;) {
    throw new Error('BANG!');
  }
} finally {
  if (item && !item.done && iterator.return) {
    // we ended prematurely, so call iterator.return if it exists
    await iterator.return();
  }
}

The readable stream implementation has the following definition for return:

  return() {
    // destroy(err, cb) is a private API.
    // We can guarantee we have that here, because we control the
    // Readable class this is attached to.
    return new Promise((resolve, reject) => {
      this[kStream].destroy(null, (err) => {
        if (err) {
          reject(err);
          return;
        }
        resolve(createIterResult(undefined, true));
      });
    });
  }

Because your destroy implementation doesn't call the callback passed to it, basically this means that there is no continuation scheduled after the call to return and the program ends because it has run out of things to do.

[gordonmleigh]

This has been fixed by nodejs/node#29176 in node 12.10.

For earlier versions I'm getting round it by piping through an echo stream first:

class EchoStream extends stream.Transform {
  constructor() {
    super({
      transform: (data, encoding, callback) => {
        callback(null, data);
      },
    });
  }
}

This provides a 'real' stream implementation for consumers.

[thejoshwolfe]

I did my best to work around the destroy() and _destroy() methods as defined by Node. This was a breaking change and is available in the major version bump yauzl 3.0.0.