nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.24k stars 542 forks source link

RetryAgent does not support throwOnError: false #3837

Open golyshevd opened 4 hours ago

golyshevd commented 4 hours ago

Bug Description

RetryAgent does not support throwOnError: false option, it always throws UND_ERR_REQ_RETRY on retries limit exceeded

Reproducible By

import { createServer } from 'node:http';

import { Agent, RetryAgent } from 'undici';

export const retryAgentBug = async (): Promise<void> => {
  const port = 3210;

  await new Promise<void>((accept, reject) => {
    const server = createServer((req, res) => {
      res.statusCode = 500;
      res.setHeader('content-type', 'text/plain');
      res.write('Internal Server Error');
      res.end();
    });

    server.once('error', reject);
    server.listen(port, accept).unref();
  });

  const dispatcher = new RetryAgent(new Agent());

  await dispatcher.request({
    method: 'GET',
    origin: `http://localhost:${port}`,
    path: '/',
    // `UND_ERR_REQ_RETRY` will still be thrown
    throwOnError: false,
  });
};

void retryAgentBug();

Expected Behavior

Expecting RetryAgent to support throwOnError option as descendant of of Dispatcher. Expecting RetryAgent.request to still return Dispatcher.ResponseData even if retries limit exceeded

Logs & Screenshots

image

Environment

Mac OS Sonoma, Node.js v18.18.2, undici@6.20.1

Additional context

Faced this issue when tried to know how to get backend response's body after N unsuccessful retries. It could be natively possible if RetryAgent was taking into account throwOnError

mcollina commented 3 hours ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

golyshevd commented 41 minutes ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

Yes I would. I think I have a solution. But right now I am trying to run tests

Idk why many tests fail even without my changes (on main branch) like that:

image

Maybe I did not set something up yet? Exactly these 76 tests fail stable for me

mcollina commented 38 minutes ago

What Node.js version are you running, what OSS are you in? Tests are passing here?

golyshevd commented 21 minutes ago

What Node.js version are you running, what OSS are you in? Tests are passing here?

node -v
v18.18.2

Tried v22 - tests passed ) Just wrongly assumed that package.engines.node is also fine for development