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

Multi-target API pain points #157

Open
aecsocket opened this issue Jul 30, 2024 · 8 comments
Open

Multi-target API pain points #157

aecsocket opened this issue Jul 30, 2024 · 8 comments

Comments

@aecsocket
Copy link
Contributor

Right now, I am having some problems with the developer experience when targeting multiple targets (wasm/native) with xwt. Even though the goal of xwt is to be cross-platform, it still feels like I have to use a lot of target-specific cfg blocks to get things done.

I'll add code examples of what my code looks like right now with target-specific cfgs, and what I would like it to look like ideally.

Initializing the endpoint in a cross-platform way is tricky

    if #[cfg(target_family = "wasm")] {
        pub type ClientEndpoint = xwt_web_sys::Endpoint;

        pub fn create_client_endpoint(config: WebTransportOptions) -> Result<ClientEndpoint, ClientError> {
            Ok(xwt_web_sys::Endpoint {
                options: config.to_js(),
            })
        }
    } else {
        pub type ClientEndpoint = xwt_wtransport::Endpoint<wtransport::endpoint::endpoint_side::Client>;

        pub fn create_client_endpoint(config: ClientConfig) -> Result<ClientEndpoint, ClientError> {
            let raw = wtransport::Endpoint::client(config).map_err(ClientError::CreateEndpoint)?;
            Ok(xwt_wtransport::Endpoint(raw))
        }
    }

Ideally

use xwt::{ClientEndpoint, ClientConfig}

// ClientConfig is a type alias for WebTransportOptions or wtransport::ClientConfig
pub fn create_client_endpoint(config: ClientConfig) -> Result<ClientEndpoint, ClientError> {
    xwt::ClientEndpoint::new(config) // -> Result<ClientEndpoint, std::io::Error or Infallible>
}

Errors received from JS are not Send + Sync

Due to the *mut u8 stored inside the JsValue. Arguably this is fine, since it is actually what is returned from the wasm-bindgen functions, but this seriously harms real-world usage. When I get a JsError, my (cross-platform) code does not care at all about the JsValue itself - it just wants a string representation of the error. So what I have to end up doing is

pub struct JsError(pub String);
impl From<xwt_web_sys::Error> for JsError {
    // extracts the error string from the xwt error
}

// on native
        type ConnectError = <internal::ClientEndpoint as Connect>::Error;
        type AwaitConnectError = <<internal::ClientEndpoint as Connect>::Connecting as xwt_core::endpoint::connect::Connecting>::Error;

// on WASM
        type ConnectError = JsError;
        type AwaitConnectError = JsError;

pub enum ClientError {
    /// Failed to connect to the target.
    #[error("failed to connect")]
    Connect(#[source] ConnectError),
    /// Failed to await the connection to the target.
    #[error("failed to await connection")]
    AwaitConnect(#[source] AwaitConnectError),
}

    #[allow(clippy::useless_conversion)] // multi-target support
    let conn = endpoint
        .connect(&target)
        .await
        .map_err(|err| ClientError::Connect(err.into()))? // have to use `.into()` here
        .wait_connect()
        .await
        .map_err(|err| ClientError::AwaitConnect(err.into()))?; // have to use `.into()` here

When this could be made so much simpler for the end user:

pub enum ClientError {
    /// Failed to connect to the target.
    #[error("failed to connect")]
    Connect(#[source] <internal::ClientEndpoint as Connect>::Error),
    /// Failed to await the connection to the target.
    #[error("failed to await connection")]
    AwaitConnect(#[source] <<internal::ClientEndpoint as Connect>::Connecting as xwt_core::endpoint::connect::Connecting>::Error),
}


    let conn = endpoint
        .connect(&target)
        .await
        .map_err(ClientError::Connect)?
        .wait_connect()
        .await
        .map_err(ClientError::AwaitConnect)?;

At the cost of losing the raw JsValue, but remember - my code is cross-platform, I'm not even guaranteed to have a JsValue on native, so I have no use for it.

Getting MTU

There's no function on Session to get max datagram size. This one is just an oversight and can probably be fixed with a quick PR.

// wasm
        #[allow(clippy::unnecessary_wraps)] // must match fn sig
        pub fn get_mtu(conn: &Connection) -> Option<usize> {
            let mtu = usize::try_from(conn.transport.datagrams().max_datagram_size())
                .expect("should be able to fit u32 into usize");
            Some(mtu)
        }

// native
        pub fn get_mtu(conn: &Connection) -> Option<usize> {
            conn.0.max_datagram_size()
        }

Handling the send/receive loop

On wtransport implementation, you are able to do code like

let conn: Connection;
let send = async move {
    loop {
        conn.send_datagram(..);
    }
};
let recv = async move {
    loop {
        process_incoming(conn.receive_datagram(..));
    }
};
let err = futures::select! {
    r = send.fuse() => r,
    r = recv.fuse() => r,
}.unwrap_err();

However, on WASM, this leads to weird behaviour where the transport can't receive as fast as it can send out. On WASM, you actually have to spawn the send/recv loops on separate wasm_bindgen_futures tasks. However, you can't split this single Connection up into its sending and receiving halves, so you can't spawn these tasks.

This behaviour is also not documented anywhere and is basically just trial and error.

If possible, I would also like to retain the Connection or at least the Rc<sys::WebTransport> after splitting the connection into sender/receiver, so that I can also periodically get the MTU of the connection and send it over a channel.

I would like code like this:

let session: Session;
let (send, recv) = session.split(); // fn split(&self) -> (SessionSender, SessionReceiver)

wasm_bindgen_futures::spawn_local(async move {
    send.send_datagram(..);
});
wasm_bindgen_futures::spawn_local(async move {
    process_incoming(recv.receive_datagram(..));
});
wasm_bindgen_futures::spawn_local(async move {
    send_mtu.send(session.max_datagram_size());
});
@MOZGIII
Copy link
Owner

MOZGIII commented Jul 31, 2024

The last issue can be solved with the current API by placing Session into Arc and providing a clone to each of the spawns; this is how the splitting would have to work either way inder the hood.

I'll comment on the rest of the issues in a bit - for now I think this solves the majn pain point you mentioned in the chat.

@aecsocket
Copy link
Contributor Author

I remembered that Arcs exist, but I completely forgot that the send and receive datagram methods take a shared ref, so I don't have to wrap it in a Mutex. Yeah, putting the Session in an Arc works great and I'm just an idiot on that front 😄

I think the other issues are still valid though. I should be able to write a PR for the MTU one quickly - only question I have is what should it be called? max_datagram_size?

@MOZGIII
Copy link
Owner

MOZGIII commented Aug 2, 2024

what should it be called

This question often drives me into a multi-hour research and design iterations... But I guess we can start with this name, yeah.

If you'll be adding it please also add the tests - sometging really simple yet that would break if the implementation changes in a way that breaks the API or logic.

@MOZGIII
Copy link
Owner

MOZGIII commented Aug 2, 2024

I think I have a somewhat elegant solution for JS errors in mind... In most cases you'd want the JS error there though.

@MOZGIII
Copy link
Owner

MOZGIII commented Sep 6, 2024

Initializing the endpoint in a cross-platform way is tricky

This is a known issue, and could be solved by providing a simpler crate that would join the wasm/native implementation together like at the example.

However, the issue is that the API, really, is not just multi-target, it is also intended to be multi-implementation. Meaning, we want to be able to switch not just between native/wasm, but also between multiple native (and wasm) implementations. For instance, we currently offer just one native implementation - wtransport-based; but there might be other implementations available, like the h3-based implementation that doesn't even use quinn under the hood. More specifically, those added implementations might be available in the future if we add them as part of this repo - but in fact they might be already present now and live in some private repos, since anyone can implement xwt-core and plug in their implementation.

This design makes the xwt use super flexible, and the intended way is to write the most of the netcode in such a way that it operates on the generic transports to be abstract around the underlying implementations.
Then, from the fn main of your app, you'd provide concrete implementations of transports and specify the generics parameters used in the rest of the netcode.

Of course, in some cases your code can't be implemented in terms of only providing pre-made transports to the majority of the netcode, like when the transports have to be created on-the-fly, this gets more complicated - but when that complexity appears you usually see the direct access to the internals as a blessing; in this case, it allows specializing the transport setup code very flexibly.

So, what am I saying with all this?

In short:

  1. having a simple API for portable instantiation of the transport is a cross-platform would have to be very opinionated in

    a) what native and wasm implementation it uses in the first place, and
    b) what configuration parameters it allows to set in those transports (as it would have to a lowest-common-subset of each transport's settings)

  2. in the simple scenarios the code for setting up the transport is either way simple and it written just once

  3. in the complicated scenarios, the flexibility and access to the internals is good; note this point has nothing to say about the actual subject of the discussion, but it is rather a tangential info explaining the current design

To sum up: I don't mind having such API, but implementing it would most likely require a new crate that would depend on both xwt-web-sys and xwt-wtransport, and provide a unified yet basic initialization routing that is portable across targets, but not implementations. Thus it would be a separate opt-in to use, and thus would be able to be as opinionated as it has to be.


In the mean time, I am back-and-forth designing with a type-erased wrapper around the arbitrary xwt-core implementation. Such that you don't have to write the code in terms of generics, but still have the flexibility of a multi-driver implementation - but much easier configurable at runtime, and also with less of the generics hassle (it is kind of annoying to deal with for error types). This wrapper would still allow low-level access to the underlying implementation through the Any trait - on a need basis, so it would be best of both worlds.

We already have a transparent wrapper xwt-anchor to provide types to attach your own custom traits over, but it might be deprecated soon as it doesn't really give much of an edge over custom newtypes - and we might want to instead provide easy-to-define newtype wrappers, like xwt-anchor but at your own crate.


This is the first part of my reply to the rest of the points brought up in this issue; I'll cover the rest later. So far, it has been very interesting, and I'm grateful for such feedback and an opportunity to share my thoughts on the concrete aspect of the design.

@MOZGIII
Copy link
Owner

MOZGIII commented Sep 6, 2024

The second, and the last topic what we didn't cover so far is this

Errors received from JS are not Send + Sync

I understand this is quite a difficult constraint, but in practice it should be possible to overcome.

From a design point of view, we'd want the errors to contain the raw JS values so that it is possible to read the values from the inner JS error - as we don't really cover the API fully just yet, and there is useful data in those errors that can be fetched directly.

Now, to the practical stuff - workarounds; with your example, you could do this:

pub struct XwtError(pub String);
impl From<<internal::ClientEndpoint as Connect>::Error> for XwtError {
    // extracts the error string from the xwt error
}
impl From<<internal::ClientEndpoint as Connect>::Connecting as xwt_core::endpoint::connect::Connecting>::Error> for XwtError {
    // extracts the error string from the xwt error
}

pub enum ClientError {
    /// Failed to connect to the target.
    #[error("failed to connect")]
    Connect(XwtError),
    /// Failed to await the connection to the target.
    #[error("failed to await connection")]
    AwaitConnect(XwtError),
}

    #[allow(clippy::useless_conversion)] // multi-target support
    let conn = endpoint
        .connect(&target)
        .await
        .map_err(XwtError::from)
        .map_err(ClientError::Connect)?
        .wait_connect()
        .await
        .map_err(XwtError::from)
        .map_err(AwaitConnect)?;

Still sort of annoying to write, but arguably better.

With new traits though, it could even easier:

pub struct XwtError(pub String);
impl From<<internal::ClientEndpoint as Connect>::Error> for XwtError {
    // extracts the error string from the xwt error
}
impl From<<internal::ClientEndpoint as Connect>::Connecting as xwt_core::endpoint::connect::Connecting>::Error> for XwtError {
    // extracts the error string from the xwt error
}

trait MapXwtErrorEx {
    type Ok;

    fn map_xwt_err<F: FnOnce(XwtError) -> U, U>(self, f: F) -> Result<Self::Ok, U>;
}

impl<T, E> MapXwtErrorEx for Result<T, E>
where
	XwtError: From<E>,
{
    type Ok = T;

    fn map_xwt_err<F: FnOnce(XwtError) -> U, U>(self, f: F) -> Result<Self::Ok, U> {
        self.map_err(XwtError::from).map_err(f)
    }
}

pub enum ClientError {
    /// Failed to connect to the target.
    #[error("failed to connect")]
    Connect(XwtError),
    /// Failed to await the connection to the target.
    #[error("failed to await connection")]
    AwaitConnect(XwtError),
}

    let conn = endpoint
        .connect(&target)
        .await
        .map_xwt_err(ClientError::Connect)?
        .wait_connect()
        .await
        .map_xwt_err(AwaitConnect)?;

This is a bit of a hack, of course - but it almost improves the ergonomics to a reasonable level for now.


Of course, we might want to, instead, figure out a way to represent those non-Send/non-Sync errors somehow; maybe an std::sync::Exclusive will help here - but I'm not sure, need to experiment.

I am looking forward to help in this regard, and we should dedicate a whole new issue just for the work on this if we are going to do it.

@MOZGIII
Copy link
Owner

MOZGIII commented Sep 6, 2024

A lot of text, but there's no rush, so please take you time 😃 I hope it is useful.

@aecsocket
Copy link
Contributor Author

I appreciate the write-up! Let me give my thoughts on this:

Initializing the endpoint in a cross-platform way is tricky

I agree with your points, and although my own crates are targeting cross-platform instead of cross-implementation, I can understand why you want to target multiple implementations for the same platform. Because of this, I'm inclined to agree with just depending on xwt-web-sys and xwt-wtransport directly. The two are very different implementations, and have different configurations, and abstracting them down into a common subset reduces the functionality severely.

For my own crate, I think this is enough justification to depend on the two implementations explicitly, and expose a pub type ClientConfig type alias for the platform-specific configuration - either a WebTransportOptions or a wtransport::ClientConfig. Still not perfectly cross-platform, but since this is only for initial configuration, I'm fine with this. Users will likely have #[cfg(target_family = "wasm")] in other places in their codebase as well.

For xwt, I also think this is enough justification to not make a common xwt crate which hard depends on a specific WASM and a specific native implementation; IMO the implementation used should be a conscious decision made by the user. But ultimately this is up to you.

About xwt-core, I am somewhat conflicted on this. On the one hand, the current generic-heavy approach does work and is a zero-cost abstraction, however it makes finding function definitions incredibly annoying since everything is a trait. I feel like a type-erased approach would be too limiting. Perhaps if type erasure is only used for errors? I'm honestly not too sure on this point without being able to use a version of the type-erased API and comparing it to the current generic-heavy API.

Ultimately, I think the best solution for xwt-core is to improve the documentation. Currently, there's not much, and adding backlinks to different parts of the API would greatly help doc navigation IMO.

Errors received from JS are not Send + Sync

Yeah I agree that there is useful data in the JS Error itself, and that users should be able to consume that raw error. But I don't think your workaround works on multi-platform: on WASM, the XwtError is fine since we have no better error type than a wrapper around a String. But on native, we do have concrete error types, for example:

type ClientEndpoint = xwt_wtransport::Endpoint<endpoint_side::Client>;
type ConnectError = <ClientEndpoint as Connect>::Error; // or XwtError on WASM
type AwaitConnectError = <<ClientEndpoint as Connect>::Connecting as Connecting>::Error; // or XwtError on WASM

The final error enum would look like:

pub enum ClientError {
    #[error("failed to connect")]
    Connect(ConnectError),
    #[error("failed to await connection")]
    AwaitConnect(AwaitConnectError),
}

I still do want to expose the raw error type on native, but just not on WASM, since that's !Send + !Sync and I only value it for its string error anyway. In this case, I couldn't use .map_err(XwtError::from), since on native I don't want to turn it into an XwtError (and in fact the XwtError would be cfg'd out on native anyway).

I don't think an Exclusive or SyncWrapper would help here, since the error has to be Send as well, and Exclusive only provides Sync. Really, I think my old solution of .map_err(|err| ClientError::Connect(err.into())) and impl From<JsError> for XwtError is the best, even if it means I need some extra clippy lint allows. Unless you can think of something smart that I haven't thought of?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants