Empty fetch() response with 'Content-Encoding: br' header causes a crash due to unhandled error event in BrotliDecompress

[ikreymer]

Version

22.6.0, 20.9.0, 20.17.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

An empty response with Content-Encoding: br results in an error if the response is canceled, not consumed, consumed by a stream.

This can be reproduced with a server:

import { createServer } from 'node:http';

createServer((req, res) => {
  res.writeHead(200, {
    "Content-Length": "0",
    "Connection": "close",
    "Content-Encoding": "br"
  });
  res.end();
}).listen(3010);

and then a client calling:

async function main() {
  const url = "http://localhost:3010/"
  try {
    const r = await fetch(url);
    console.log("done");
  } catch (e) {
    console.log(e);
  }
}

main();

Although done will be printed, an exception will be thrown afterwards:

done
node:events:498
      throw er; // Unhandled 'error' event
      ^

Error: unexpected end of file
    at genericNodeError (node:internal/errors:983:15)
    at wrappedFn (node:internal/errors:537:14)
    at BrotliDecoder.zlibOnError [as onerror] (node:zlib:191:17)
Emitted 'error' event on BrotliDecompress instance at:
    at emitErrorNT (node:internal/streams/destroy:170:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -5,
  code: 'Z_BUF_ERROR'
}

How often does it reproduce? Is there a required condition?

This happens whenever the stream is not immediately consumed

The following will cause a crash:

const r = await fetch("http://localhost:3010/");
console.log("done");

const r = await fetch(url);
await r.body.cancel();
console.log("done");

const r = await fetch(url);
const reader = r.body.getReader()
console.log("done");

The following will NOT cause a crash, because the body is immediately consumed:

const r = await fetch(url);
await r.text();
console.log("done");

const r = await fetch(url);
const reader = r.body.getReader();
while (true) {
  const { value, done } = await reader.read();
  if (done) {
    break;
  }
}

What is the expected behavior? Why is that the expected behavior?

fetch() should not a cause an unhandled error that crashes node, even if the response body is never consumed, especially since it is empty. This is the behavior with other compression types, such as Content-Encoding: gzip.

What do you see instead?

fetch() causes node to crash due to an unhandled error event from BrotliDecompress

Additional information

No response

[KhafraDev]

@ronag it seems like pipeline isn't forwarding the error here? This fixes it:

diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js
index 435eafdf..9a26977e 100644
--- a/lib/web/fetch/index.js
+++ b/lib/web/fetch/index.js
@@ -2153,7 +2153,7 @@ async function httpNetworkFetch (
               } else if (coding === 'deflate') {
                 decoders.push(createInflate())
               } else if (coding === 'br') {
-                decoders.push(zlib.createBrotliDecompress())
+                decoders.push(zlib.createBrotliDecompress().on('error', noop))
               } else {
                 decoders.length = 0
                 break

[KhafraDev]

import { pipeline, Readable } from 'node:stream'
import { createBrotliDecompress } from 'node:zlib'

pipeline(new Readable({
  read () {
    this.push(null)
  }
}), createBrotliDecompress(), () => {})

reproducible without fetch

[ronag]

That example is incorrect as pipeline will return a readable stream:

pipeline(new Readable({
  read () {
    this.push(null)
  }
}), createBrotliDecompress(), () => {}).on('error, () => {})

[ronag]

it seems like pipeline isn't forwarding the error here? This fixes it:

As far as I can tell it is forwarding it, it's just not handled...

[ronag]

Seems to me related to either fetch or WebStreams.

[KhafraDev]

As far as I can tell it is forwarding it, it's just not handled...

Shouldn't the error be passed to the callback (last parameter)? Maybe I'm misunderstanding it?

This example is what I'm confused about: https://nodejs.org/api/stream.html#streampipelinestreams-callback

[ronag]

As far as I can tell it is forwarding it, it's just not handled...

Shouldn't the error be passed to the callback (last parameter)? Maybe I'm misunderstanding it?

This example is what I'm confused about: https://nodejs.org/api/stream.html#streampipelinestreams-callback

Not if the last stream passed is readable.

[Uzlopak]

provided PR #3620