Skip to content

fix: Report all transport errors in a dial attempt #5899

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

rose2221
Copy link

@rose2221 rose2221 commented Mar 1, 2025

Description

Currently, when a DNS address resolves to multiple IPs, the transport dials each one, but only the last error is returned, while earlier errors are logged and lost. This causes issues like #5871, where users miss crucial failure details.

This PR aggregates all transport errors from failed dial attempts and returns them, similar to DialError::Transport at the swarm level.

Fixes

Fixes #5878
Fixes #5871

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@rose2221 rose2221 changed the title dns: Report all transport errors in a dial attempt fix: Report all transport errors in a dial attempt Mar 1, 2025
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Left some suggestions. Can you bump the minor version to 0.44.0 since this is introducing a breaking change and 0.43 has already been released? :)

@dariusc93 dariusc93 requested review from jxs and elenaf9 March 1, 2025 15:39
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rose2221, this already looks good. Just some nits.

@rose2221
Copy link
Author

rose2221 commented Mar 3, 2025

Thanks @rose2221, this already looks good. Just some nits.

Thanks, @dariusc93 and @elenaf9, for the feedback! I've made the requested updates. Let me know if any further refinements are needed!

Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also bump the crate version in Cargo.toml to 0.44 as well as in the workspace Cargo.toml? :)

@rose2221
Copy link
Author

rose2221 commented Mar 3, 2025

Could you also bump the crate version in Cargo.toml to 0.44 as well as in the workspace Cargo.toml? :)

it's been done!

@rose2221 rose2221 requested a review from elenaf9 March 5, 2025 09:29
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks good to me. Just a few nits about the test.

@rose2221 rose2221 requested a review from elenaf9 March 21, 2025 05:45
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow replies, my availability is a bit limited.
Few follow-up comments.

@rose2221 rose2221 requested a review from elenaf9 April 3, 2025 13:33
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the follow ups @rose2221.

Last nit, then only formatting and fixing the clippy beta lints is needed.

rose2221 and others added 2 commits April 4, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants