BiagioFesta / wtransport

Async-friendly WebTransport implementation in Rust
Apache License 2.0
470 stars 31 forks source link

`reset` error type #221

Closed MOZGIII closed 2 months ago

MOZGIII commented 2 months ago

I'm a bit disappointed by the new API for the reset fn.

Currently, one has to handle NotConnected, Stopped, QuicProto and Closed enum variants there, while the only real possible failure condition is Closed. This is a serious obstacle to the proper error handling type system, and given how the rest of the errors follow the principle of "caller only has to handle failures that can actually happen from the call" it seems just an oversight.

So, let's fix it?

BiagioFesta commented 2 months ago

Yeah I agree on the concern.


We have that a "write" might also lead to a CloseStream error, thus, that requires keeping Closed enum variant and duplicate into another one to have a dedicated type. Is there a rust-idiomatic way to do that?

Solution 1 (Kinda-Type duplication)

#[error]
pub struct ClosedStream;

#[error]
pub enum WriteError {
  // ...
  ClosedStream,
  // ...
}

That's the solution also adopted by quinn; probably the preferable.

Solution 2 (not very rust idiomatic)

/// Returns `true` if stream reset; otherwise `false` is already closed.
pub fn reset(&self) -> bool {
  // ...
}

This avoid the "enum duplication"; but honestly I don't like it. Not "rusty", not idiomatic (not very talkative interface). I'd discard that.

Solution 3 ?

Proposals?

MOZGIII commented 2 months ago

Solution 1 is the way to go, it is the cleanest in my opinion.

Type duplication is actually no happening there, as the types specifically are not duplicated - we got two types that exactly describe the conditions the corresponding two calls can produce; the fact that one of them describes the same condition in on of the variants as the whole other type is not an issue, since this is precisely how the reality of the world we are describing is.

Note there is no need to embed the ClosedStream type into the WriteError::ClosedStream variant either, since this is meaningless, and any conversion you want to implement between the two that would seemingly be facilitated by this can also be implemented without it; and, also, we don't need such conversions in the first place, as there are ways to better do the error type unification.

MOZGIII commented 2 months ago

Note that for xwt I'm adding a layer akin to solution 2 on top of the raw errors at the driver layer - but that's because we model the high level interesting condition on top of the existing types... But that is actually something that we might revisit at a later design phase in the xwt (with a high-level interface atop of low-level drivers). So it is for different reasons, and for wtransport solution 1 makes sense, while solution 2 doesn't imo.

BiagioFesta commented 2 months ago

Yeah I agree, I was just curious whether there'd exist an even better[1] solution

[1]: better means nothing, but let's say: "more idiomatic"