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

RetryAgent does not support throwOnError: false #3837

Open
golyshevd opened this issue Nov 15, 2024 · 5 comments
Open

RetryAgent does not support throwOnError: false #3837

golyshevd opened this issue Nov 15, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@golyshevd
Copy link

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, [email protected]

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

@golyshevd golyshevd added the bug Something isn't working label Nov 15, 2024
@mcollina
Copy link
Member

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

golyshevd added a commit to golyshevd/undici that referenced this issue Nov 15, 2024
@golyshevd
Copy link
Author

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
Copy link
Member

mcollina commented Nov 15, 2024

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

@golyshevd
Copy link
Author

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

@golyshevd
Copy link
Author

In main branch (version: 7.0.0-alpha.3) throwOnError option was dropped :D
Now undici never throws http errors

Does it mean now that RetryAgent should also never throw an error even if http "error" was retried maxRetries times?

I think the issue is not a bug now, but feature request. I still need to get response after retries did not lead to success

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

No branches or pull requests

2 participants