feat(iroh-relay)!: use concrete errors for the client part of iroh-relay #3161

dignifiedquire · 2025-02-03T11:19:30Z

Description

Another step towards #2741

Breaking Changes

RelayMap::from_nodes does not return an error anymore
errors in client apis of iroh_relay are not anyhow::Error but rather concrete error types

Notes & open questions

Change checklist

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

dignifiedquire · 2025-02-03T11:20:31Z

iroh-relay/src/client.rs

@@ -37,6 +37,48 @@ pub(crate) mod streams;
 #[cfg(not(wasm_browser))]
 mod util;

+/// Client related errors
+#[allow(missing_docs)]


I don't think there is much value in documenting these variants currently, which is why I am skipping them

github-actions · 2025-02-03T11:21:39Z

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

Last updated: 2025-02-18T09:23:04Z

github-actions · 2025-02-03T11:33:19Z

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: 2e749a3

flub

I think fundamentally I'm not happy with these giant enum errors. I think at every point an error can be returned, each error variant of the type must be possible. The approach here just makes every source error a variant without any concern on whether this really helps anything.

flub · 2025-02-03T14:01:46Z

iroh-relay/src/client.rs

+#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum Error {
+    #[error("Invliad target port")]
+    InvalidTargetPort,
+    #[error("Invalid URL {0}")]
+    InvalidUrl(Url),
+    #[error("Invalid Proxy URL {0}")]
+    InvalidProxyUrl(Url),
+    #[error("Invalid URL for websocket {0}")]
+    InvalidWebsocketUrl(Url),
+    #[error(transparent)]
+    Websocket(#[from] tokio_tungstenite_wasm::Error),
+    #[cfg(not(wasm_browser))]
+    #[error(transparent)]
+    Dns(#[from] DnsError),
+    #[error(transparent)]
+    Hyper(#[from] hyper::Error),
+    #[error(transparent)]
+    InvalidDnsName(#[from] rustls::pki_types::InvalidDnsNameError),
+    #[error("Unexpected status during upgrade: {0}")]
+    UnexpectedUpgradeStatus(hyper::StatusCode),
+    #[error("Failed proxy connection: {0}")]
+    FailedProxyConnection(hyper::StatusCode),
+    #[error(transparent)]
+    ProtoRelay(#[from] crate::protos::relay::Error),
+    #[error(transparent)]
+    Io(#[from] std::io::Error),
+    #[error("Timeout")]
+    Timeout(#[from] time::Elapsed),
+    #[error(transparent)]
+    Http(#[from] hyper::http::Error),
+    #[error("Unexpected frame received {0}")]
+    UnexpectedFrame(crate::protos::relay::FrameType),
+    #[error(transparent)]
+    Utf8(#[from] std::str::Utf8Error),
+    #[cfg(wasm_browser)]
+    #[error("The relay protocol is not available in browsers")]
+    RelayProtoNotAvailable,
+}


I'm afraid I don't like this kind of error. I think I even just removed this kind of error from here. It just isn't useful as a user of this. Almost all of these are just a message in practice. Like who knows whether they should handle a Hyper error variant in this? No clue.

Also the places where this error is returned can never return every branch of this enum. I think this should be a useful guiding principle: every enum error returned should be able to take every value in that enum at each point where it is used. Otherwise it is strictly worse than using anyhow.

that's fair, I guess I have more work to do

@flub please take another look, while certainly not perfect, I think this is a good improvement over the status quo

flub · 2025-02-03T14:01:54Z

iroh-relay/src/client.rs

@@ -216,7 +258,7 @@ impl Client {
 }

 impl Stream for Client {
-    type Item = Result<ReceivedMessage>;
+    type Item = Result<ReceivedMessage, Error>;


This is not useful, this error type has a whole bunch of variants which can not happen here. Like almost all of them. What can happen here are the variants of ConnSendError. And they are a useful subset to distinguish between. I would have liked to rename ConnSendError into ConnError and use it here as well, but that was a lot of churn to make it work and I had to concentrate on the other logic changes at the time.

matheus23

I'm having a little bit of trouble evaluating this PR. Not sure how to decide what kinds of errors are good / how to best group them etc.

I liked the change towards .expect in some places, where it'd really just be a bug in our code if there were an error.

Anyhow - given we want to move forward with this, we should get this into main. I can see some git conflicts on the horizon for two of my WIP branches 🙃

dignifiedquire · 2025-02-18T09:07:05Z

I'm having a little bit of trouble evaluating this PR. Not sure how to decide what kinds of errors are good / how to best group them etc.

I think this is something we should explore, there is no fixed rule that works for every project I have found so far.

flub

I am struggling to figure out how to be constructive on this. I still don't particularly like this error style. I think it does not think enough about the human consumer. The error variants inside single enums are often of widely different "types".

OTOH I don't feel like particularly holding this up. Though I am a little worried if it becomes the model we use everywhere. If I were to properly handle this I'd create a counter-proposal as PR... but that takes time. Which I guess is part of the question, my approach would be a much more time-intensive I expect as it really asks for a deep understanding of each API.

Anyway, eh, feel free to merge?

flub · 2025-02-18T09:56:17Z

iroh-base/src/key.rs

@@ -9,8 +9,8 @@ use std::{
 };

 use curve25519_dalek::edwards::CompressedEdwardsY;
-pub use ed25519_dalek::Signature;
-use ed25519_dalek::{SignatureError, SigningKey, VerifyingKey};
+pub use ed25519_dalek::{Signature, SignatureError};


Will there be any regrets about re-exporting these without wrapping?

flub · 2025-02-18T10:04:54Z

iroh-relay/src/client.rs

+    #[error("Invalid relay URL {0}")]
+    InvalidRelayUrl(Url),
+    #[error(transparent)]
+    Websocket(#[from] tokio_tungstenite_wasm::Error),


I'm not sure this error is self-describing. Maybe an #[error("Websocket transport error: {0}")] would be better? (If that is the correct interpretation)

Which immediately makes me wonder "but what if I'm not using websocket"? Why is there no generic IO or Transport error this falls under?

flub · 2025-02-18T10:07:25Z

iroh-relay/src/client.rs

+    InvalidTlsServername,
+    #[error("No local address available")]
+    NoLocalAddr,
+    #[error("tls connection failed")]


Suggested change

#[error("tls connection failed")]

#[error("TLS connection failed")]

flub · 2025-02-18T10:17:03Z

iroh-relay/src/client.rs

+#[allow(missing_docs)]
+#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum DialError {


Reading just the error types I am rather confused what the difference between ConnectError and DialError is. Sounds like the same to me.

It seems DialError is a variant of ConnectError so maybe it's more of a ConnectionError? But then also ConnectError is returned from Client::connect so it probably was meant as ConnectError 😵‍💫

flub · 2025-02-18T10:25:44Z

iroh-relay/src/client/conn.rs

+    #[error(transparent)]
+    Websocket(#[from] tokio_tungstenite_wasm::Error),
+    #[error("invalid protocol message encoding")]
+    InvalidProtocolMessageEncoding,
+    #[error("Unexpected frame received {0}")]
+    UnexpectedFrame(crate::protos::relay::FrameType),


Websocket seems like part of the Io variant?
And InvalidProtocolMessageEncoding and UnexpectedFrame are part of the Protocol variant?

I think the io/protocol distinction is pretty decent on a high-level and that was what I was going for in the ConnSendError.

Also it might be possible to have ConnSendError and RecvError be the same type?

dignifiedquire · 2025-02-18T14:52:50Z

Unfortunately I found a massive drawback to using thiserror type errors. You loose backtrace information for most errors that are wrapped, including std errors. This results in quite an awful debugging experience, especially in a test runner setup.

The reasons for this is that tldr rust-lang/rust#99301 is not stable yet and error libraries can't propagate backtrace information properly. This works with anyhow because it uses a singular error type, which can poperly propagate backtraces.

Until I can find a solution for this, using error enums is for now unacceptable.

If you ask, Yes I probably looked at your favourite error handling library, they all fail to do this on STABLE rust, including

eyre and it's various reporters
thiserror
snafu
anyhow combined with thiserror or snafu
testresult

dignifiedquire commented Feb 3, 2025

View reviewed changes

dignifiedquire requested review from flub and matheus23 February 3, 2025 11:24

matheus23 changed the title ~~feat(iroh-relay)!: use concret errors for the client part of iroh-relay~~ feat(iroh-relay)!: use concrete errors for the client part of iroh-relay Feb 3, 2025

flub requested changes Feb 3, 2025

View reviewed changes

dignifiedquire marked this pull request as draft February 3, 2025 17:13

dignifiedquire force-pushed the feat-relay-errors branch from 96176a3 to 81f24ed Compare February 17, 2025 13:15

dignifiedquire marked this pull request as ready for review February 17, 2025 19:43

matheus23 approved these changes Feb 18, 2025

View reviewed changes

dignifiedquire added 7 commits February 18, 2025 10:07

wip: concrete errors for iroh-relay

3319c6d

finish

f910055

fixup wasm imports

ada30d7

update for latest changes

6219eb3

introduce ConnectError

d462a9e

introduce client::RecvError

1c63ae0

remove dead code

a4437f9

dignifiedquire force-pushed the feat-relay-errors branch from 74d55e7 to a4437f9 Compare February 18, 2025 09:07

wasm cfg

e32d32a

flub reviewed Feb 18, 2025

View reviewed changes

dignifiedquire marked this pull request as draft February 18, 2025 14:48

dignifiedquire mentioned this pull request Feb 18, 2025

switch to using custom error types, rather than generic anyhow::Error #2741

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-relay)!: use concrete errors for the client part of iroh-relay #3161

feat(iroh-relay)!: use concrete errors for the client part of iroh-relay #3161

dignifiedquire commented Feb 3, 2025

dignifiedquire Feb 3, 2025

github-actions bot commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

flub left a comment

flub Feb 3, 2025

dignifiedquire Feb 3, 2025

dignifiedquire Feb 17, 2025

flub Feb 3, 2025

matheus23 left a comment

dignifiedquire commented Feb 18, 2025

flub left a comment

flub Feb 18, 2025

flub Feb 18, 2025

flub Feb 18, 2025

flub Feb 18, 2025

flub Feb 18, 2025

dignifiedquire commented Feb 18, 2025

	#[error("tls connection failed")]
	#[error("TLS connection failed")]

feat(iroh-relay)!: use concrete errors for the client part of iroh-relay #3161

Are you sure you want to change the base?

feat(iroh-relay)!: use concrete errors for the client part of iroh-relay #3161

Conversation

dignifiedquire commented Feb 3, 2025

Description

Breaking Changes

Notes & open questions

Change checklist

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025 • edited Loading

github-actions bot commented Feb 3, 2025 • edited Loading

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheus23 left a comment

Choose a reason for hiding this comment

dignifiedquire commented Feb 18, 2025

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Feb 18, 2025

github-actions bot commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading