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

undici.stream ignores throwOnError #1753

Closed
mdashlw opened this issue Nov 4, 2022 · 4 comments · Fixed by #1995
Closed

undici.stream ignores throwOnError #1753

mdashlw opened this issue Nov 4, 2022 · 4 comments · Fixed by #1995
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@mdashlw
Copy link

mdashlw commented Nov 4, 2022

Bug Description

undici.stream/Dispatcher.stream accepts RequestOptions which extends DispatchOptions which includes throwOnError to throw on 4xx and 5xx HTTP errors, but completely ignores it and calls the factory regardless of the status code.

Reproducible By

import fs from "node:fs";
import undici from "undici";

await undici.stream(
  "https://httpbin.org/status/502",
  { throwOnError: true },
  () => fs.createWriteStream("httpbin_502.txt")
);

Successfully creates an empty httpbin_502.txt (because httpbin.org sends empty body on /status)

Expected Behavior

ResponseStatusCodeError: Response status code 502: BAD GATEWAY

Additional context

@mdashlw mdashlw added the bug Something isn't working label Nov 4, 2022
@ronag
Copy link
Member

ronag commented Nov 4, 2022

PR welcome!

@ronag
Copy link
Member

ronag commented Nov 4, 2022

Should do the same on undici.pipeline

@MattBidewell
Copy link
Contributor

If no one else is looking, I've started to take a look at this. :) Have a WIP draft PR here, not finished yet.

#1766

@assalielmehdi
Copy link

Hi, I implemented a fix. Can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants