Use `thiserror` instead of `anyhow` for `DisconnectReason::ByError`

[vladbat00]

It would be great to be able to match by an error type without the need to downcast errors from anyhow::Error.

Atm, it's impossible to know the range of possible errors in DisconnectReason::ByError, which makes it difficult for users to make an informed decision on what they should handle or not.
It's also a better practice for libraries to expose errors as matchable enums, whereas anyhow is advised to be used only in applications or private modules when users don't care about specific error types.

A similar issue was originally opened in lightyear, as it inherits the same problem of being unable to match by exact error type, and then also exaggerates it by casting anyhow::Error to a string via debug-printing cBournhonesque/lightyear#1704. But to fix the problem altogether in lightyear, I hope this issue can also be addressed on the aeronet end.

[aecsocket]

The problem is that we genuinely don't know the range of possible errors that could cause a disconnection. It could be because of a network link error, a silent disconnect, a DoS attempt, or if the IO is particularly picky, Mercury being in retrograde. This is info that only the IO implementation and transport layers have - not the IO abstraction layer.

What might be more useful is an enum like

enum DisconnectReasonKind {
    IoFailure(anyhow::Error),
    TransportFailure(anyhow::Error),
    User(anyhow::Error)
}

But even that would have to fallback to anyhow errors.

Also we should probably be using an Arc error instead of plain one so you can share it.

[vladbat00]

Yeah I think the point I'm trying to challenge is the need to have anyhow::Error at all. Maybe I'm wrong here, so I'll appreciate if you help me understand :)

I found there are the following code paths that construct DisconnectReason::ByError:

Details

./crates/aeronet_channel/src/lib.rs:                Some(DisconnectReason::by_error(ChannelDisconnected))
./crates/aeronet_transport/src/send.rs:                reason: DisconnectReason::by_error(err),
./crates/aeronet_transport/src/lib.rs:                reason: DisconnectReason::by_error(MemoryLimitExceeded {
./crates/aeronet_websocket/src/session/backend.rs:                    DisconnectReason::by_error(SessionError::Closed(event.code()))
./crates/aeronet_websocket/src/session/backend.rs:            _ = tx_dc_reason.try_send(DisconnectReason::by_error(err));
./crates/aeronet_webtransport/src/session.rs:                    DisconnectReason::by_error(SessionError::FrontendClosed)
./crates/aeronet_webtransport/src/session.rs:        DisconnectReason::by_error(err)
./crates/aeronet_webtransport/src/session.rs:            err => DisconnectReason::by_error(err),
./crates/aeronet_webtransport/src/server/mod.rs:                reason: DisconnectReason::by_error(SessionError::MtuTooSmall(err)),
./crates/aeronet_webtransport/src/client/mod.rs:                reason: DisconnectReason::by_error(SessionError::MtuTooSmall(err)),
./crates/aeronet_steam/src/session.rs:                reason: DisconnectReason::by_error(SessionError::InvalidConnection),
./crates/aeronet_steam/src/session.rs:                reason => DisconnectReason::by_error(SessionError::Ended(reason)),
./crates/aeronet_steam/src/session.rs:                    reason: DisconnectReason::by_error(SessionError::InvalidConnection),
./crates/aeronet_steam/src/session.rs:                    reason: DisconnectReason::by_error(SessionError::ProblemDetectedLocally),
./crates/aeronet_steam/src/client.rs:                    reason: DisconnectReason::by_error(err),
./crates/aeronet_steam/src/client.rs:                    reason: DisconnectReason::by_error(SessionError::BackendClosed),
./crates/aeronet_steam/src/client.rs:                reason: DisconnectReason::by_error(SessionError::Steam),

So far I found the following variants that could be expressed as an enum:

enum DisconnectError {
  ChannelDisconnected,
  MemoryLimitExceeded,
  TransportSendError(TransportSendError),
}

Maybe some of the errors expose implementation details too much and don't matter to users, so I don't mind if some of these variants are mapped to fewer ones.

There are also transport-specific functions that return Result<_, DisconnectReason>, where errors are first mapped to ClientError, SessionError (maybe there's more) and then eventually converted to anyhow::Error via the ? operator.

These types are transport-specific, so I was thinking whether we could create an enum wrapping each supported transport:

pub enum TransportError {
  #[cfg(feature = "steam")]
  Steam(...),
  #[cfg(feature = "websocket")]
  WebSocket(...),
  ...
}

But after writing this I realised that the highlevel aeronet crates don't depend on the transport crates. Which makes perfect sense, I guess that allows for better flexibility, having 3rd party transport crates, etc.

Still, as a user, I can't easily get an understanding to what types exactly I can downcast transport-specific errors without studying the code.

I like your suggestion to introduce some grouping for error kinds

enum DisconnectReasonKind {
    IoFailure(anyhow::Error),
    TransportFailure(anyhow::Error),
    User(anyhow::Error)
}

But instead of storing anyhow::Error there, maybe we could introduce custom error traits for each of these variants, requiring transports to implement them? For example:

pub trait TransportError: Error { }

enum DisconnectError {
  ChannelDisconnected,
  MemoryLimitExceeded,
  TransportSendError(TransportSendError),
  TransportError(Box<dyn TransportError>),
}

The main benefit for me as a user would be easier discoverability of these errors. First, it'll require transport crates to wrap low-level io/transport errors into a new type (thanks to the orphan rule), also promoting better errors structure, grouping them under an enum, etc.

Because, right now, too many different types can end up in DisconnectReason::ByError, since the only requirement for them is to be convertible to anyhow::Error.

Then, it should be easier to find types that will implement the custom error traits and at least know to which types a user can downcast to (it should even be possible to search for trait implementations via docs.rs).

[aecsocket]

So far I found the following variants that could be expressed as an enum

New first- or third-party IO layer implementations may expose new failures modes which aeronet can't know about - that's my point about not being able to know the range of errors.

so I was thinking whether we could create an enum wrapping each supported transport

Yeah, as you pointed out aeronet_io is left IO-agnostic, and it does not tie itself to any specific IO layer intentionally to allow people to build their own without needing special aeronet_io integration. This is a very important property for me, and is the direction that e.g. winit is also moving towards.

as a user, I can't easily get an understanding to what types exactly I can downcast transport-specific errors

This is a double edged sword I guess. It gives implementations more freedom, but consumers lose out on error ergonomics. But it's better to be explicit about "we don't know what kind of error this is - it's your responsibility to downcast it". Maybe the solution here is better documentation.

But instead of storing anyhow::Error there, maybe we could introduce custom error traits for each of these variants, requiring transports to implement them?

I'm hesitant on the idea of having implementations implement a trait, since the trait doesn't convey any real functionality, and I haven't seen this approach in other crates before. Again, I'd probably just do better docs - although I can see the appeal in being able to search by implementors to find all the error types for a particular variant.

introduce some grouping for error kinds

I think this is the best way forward, since it provides kinda more fine-grained errors, but still lets implementations convey specific information about an error. But I am still very adamant on keeping anyhow::Error around in the stack, at least inside an Arc on the leaf error variants. Otherwise, people will be forced to throw more specific error details away, which I absolutely don't want transports and IO layers to do. It's much more useful to have

failed to connect to server:
- starting WebTransport client
- performing connection handshake
- invalid packet frame
- expected 0xDEAD but found 0xBEEF

instead of

failed to connect to server:
- transport error
- failed to open connection

(and keep in mind, you can always throw away details in the UI if your users shouldn't care about it; but you can't re-add details that you've thrown away)

---

tldr, I'd like to find out more about your use-case, and exactly how you want to handle different error types (different UI message? retry behaviour?); then I can look into what exact error groups we'd need. I'm imagining the API will look something like:

enum DisconnectReason {
    ByError(DisconnectError)
}

// dioxus uses this pattern too!
type CapturedError = Arc<anyhow::Error>;

enum DisconnectError {
    // I don't really know what the categories should look like here
    Io(CapturedError),
    TransportSend(CapturedError),
    TransportRecv(CapturedError),
}

[vladbat00]

Thank you for the detailed response! All your points make total sense to me.

Again, I'd probably just do better docs - although I can see the appeal in being able to search by implementors to find all the error types for a particular variant.

Yeah, that would also work. It solves the problem, doesn't introduce any technical complexity, so why not. :)
I just like that with introducing a separate trait the implementations will be required to use it, which eliminates the problem of forgetting to document or update documentation after introducing more error types.

since the trait doesn't convey any real functionality, and I haven't seen this approach in other crates before

I definitely saw some projects doing empty "marker" traits that just require an implementation of other trait, not for this particular use-case, but it will probably be difficult to find the exact error pattern among other crates indeed.

tldr, I'd like to find out more about your use-case, and exactly how you want to handle different error types (different UI message? retry behaviour?); then I can look into what exact error groups we'd need

I'd like to be able to know when to do one of the following:

• retry a connection
• straight display an error that a server is unavailable
• report an error to monitoring tools if it's a client error (i.e. any implementation issue that needs investigating)

[aecsocket]

I'd like to be able to know when to do one of the following

Ah this is kinda really hard to do. There isn't really a good way to know whether a network error is transient or permanent, that's the curse of network programming. Even something like a No route to host isn't necessarily an error you want to retry or instantly fail for. If your router isn't working, it could be transient (if your router just has its uplink broken for a bit) or permanent (your router is misconfigured) or requires user action (you're behind a WiFi captive portal and need to log in).

You'll probably also want to vary behaviour depending on why you're attempting this connection. If it's a server listing and you just want to check if a server is reachable, it probably suffices to try once, or retry maybe 5 times, once per 2 sec (these are all ballpark numbers). If your player got disconnected from a game, then you should take a different approach - keep trying to connect constantly (maybe with exponential backoff), since they might be in the middle of clutching a 1v5 and it would suck if lost because of a transient network error which you could easily recover from. Basically, I don't think "you should always retry this error variant but never retry this one" - it depends on why you're trying to connect instead.

Also, a client error is something you won't be able to catch by just reading error states. An Err indicates a known fallible state, and it is 100% intended for an Err to occur in the lifetime of a network connection. If your protocol expects a 0xA in the next packet but it gets an 0xB, and you get an Err(InvalidPacket) or something, then it's just as likely to be a server error than a client error. In this case, you'd be better off logging every error (apart from known-valid errors like timeout or no route to host or whatever), then looking at the statistical distribution of errors to see what unexpected errors come up.

This isn't to say that implementation issues and bugs can't happen, but those will be caught by 1) panics (which your monitoring should be logging in 100% of cases), and 2) statistically abnormal amount of specific error kinds appearing. There is intentionally no Err(ImplementationBug), because that must either be a panic (state unreachable by the endpoint alone), or another Err variant (invalid state caused by the peer).

I definitely saw some projects doing empty "marker" traits that just require an implementation of other trait

Do you have some examples? I'm curious to see how they do it. I'd like to avoid a marker trait if possible since Arc<anyhow::Error> doesn't allow you to restrict only capturing errors implementing a specific trait, and I'd rather not have an Arc<dyn IoError> since that's less ergonomic than anyhow::Error (is a fat pointer instead of an anyhow::Error being one pointer, and does not log error messages as nicely)