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

fix(iroh): Return error if disco send via relay fails #3130

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

flub
Copy link
Contributor

@flub flub commented Jan 14, 2025

Description

The signature of this function expects an error if the send fails, but
the relay part did not do this. The impact of this is that you might
silently drop a packet, which makes debugging a bit harder. Returning
an error should propagate this correctly and the failures will be
noticed and logged.

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

The signature of this function expects an error if the send fails, but
the relay part did not do this.  The impact of this is that you might
silently drop a packet, which makes debugging a bit harder.  Returning
an error should propagate this correctly and the failures will be
noticed and logged.
Copy link

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3130/docs/iroh/

Last updated: 2025-01-14T11:53:43Z

Copy link

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: a01a141

@matheus23
Copy link
Contributor

Looking through this part of the code I'm realizing some unrelated things that could be fixed:

  • It looks like there exists MagicSock::handle_ping_actions and it's apparently unused (but... rustc doesn't realize apparently due to the #[instrument] macro)
  • Instead of returning a bool from send_disco_message_relay we should probably just return the error we get from try_send_relay, and later .map_err, so we don't forget such stuff in the future. Even Result<(), ()> > bool.

@flub
Copy link
Contributor Author

flub commented Jan 14, 2025

Looking through this part of the code I'm realizing some unrelated things that could be fixed:

* It looks like there exists `MagicSock::handle_ping_actions` and it's apparently unused (but... rustc doesn't realize apparently due to the `#[instrument]` macro)

* Instead of returning a bool from `send_disco_message_relay` we should probably just return the error we get from `try_send_relay`, and later `.map_err`, so we don't forget such stuff in the future. Even `Result<(), ()>` > `bool`.

Yeah... this code hasn't yet seen a proper cleanup.

Also, sad that #[instrument] does that, but I probably shouldn't be surprised.

@flub flub enabled auto-merge January 14, 2025 14:16
@flub flub added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit 35af23e Jan 14, 2025
27 checks passed
@matheus23 matheus23 deleted the flub/relay-send-err branch January 14, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants