feat: centralize Error enum

[ssddOnTop]

Currently, rmcp crate is throwing errors randomly, like JoinErro, std::io::Error, ClientInitializeError<std::io::Error>, SseTransportError<reqwest::Error>, ClientInitializeError<SseTransportError<reqwest::Error>>, rmcp::ServiceError, etc.

So as a user it's very difficult for me to write maintainable code without having to use something like anyhow.

So I think ideally there should be an enum rmcp::Error which is used across the library to maintain uniformity.

[4t145]

We may have different error types for different transport types, so I guess if there's a unified error, it would still be something with anyhow::Error or Box<dyn Error>. Maybe we can map the underlying transport error into an dynamic error type and unified the others error into one error type?

enum Error<TransportError = Box<dyn StdError + Send + Sync>> {
    // ...every possible error types
}

[ssddOnTop]

I think, the errors should not be generic.

For example:

If I don't want to use the default retry strategy provided by the crate, then I should be able to determine type of error to be able to retry.

So maybe we can just have an enum of errors, something like:

#[derive(thiserror::Error, Debug, derive_more::From)]
pub enum Error {
    #[error("{0}")]
    McpOperationFailed(rmcp::ServiceError),

    #[error("Unable to wait for operation completion: {0}")]
    JoinError(tokio::task::JoinError),

    #[error("IO error: {0}")]
    IoError(std::io::Error),

    #[error("Stdio client initialization error: {0}")]
    StdioInitError(ClientInitializeError<std::io::Error>),

    #[error("SSE transport error: {0}")]
    SseError(SseTransportError<reqwest::Error>),

    #[error("SSE client initialization error: {0}")]
    SseInitError(ClientInitializeError<SseTransportError<reqwest::Error>>),
}

Also, I feel like we should not have wrappers for Errors like: ClientInitializeError<SseTransportError<reqwest::Error>> but that would be another issue.

About your concern:

We may have different error types for different transport types

Maybe we can create an enum for errors in different transport types, i.e. we can just use the existing SseTransportError and wrap it in the main Error enum.

So as a user I can at least pattern match the errors and I won't have to create custom enum on my end to handle those errors.

Potential question(s):

1. Why enum instead of Box<dyn Error> ?
   Ans: If someone wants to know type of error, then it's very difficult to find/guess the type of error, so enum approach will help users figure type of errors.

[4t145]

Maybe we can create an enum for errors in different transport types, i.e. we can just use the existing SseTransportError and wrap it in the main Error enum.

You cannot include the customized tranport's error in it. (There's one ongoing https://gitlab.com/lx-industries/rmcp-actix-web). So maybe we can wrap all transport error into one transport error with a tag of transport type, witch could be transport type's name by using std::any::type_name_of, or provided by a method of the transport trait. And then, we can reduce all different init error and transport error into two or three variants.

#[derive(thiserror::Error, Debug, derive_more::From)]
pub enum Error {
    #[error("{0}")]
    McpOperationFailed(rmcp::ServiceError),

    #[error("Unable to wait for operation completion: {0}")]
    JoinError(tokio::task::JoinError),

    #[error("...")]
    ClientInitializeError(ClientInitializeError),

    #[error("...")]
    ServerInitializeError(ServerInitializeError),

    #[error("...")]
    TransportError(TransportError),
}

[ssddOnTop]

You cannot include the customized tranport's error in it.

I am not much familiar with the code base and how you are planning to incorporate new transports.

So my understanding for this issue is that let's say there's a new transport, or just a new implementation of existing transport, the errors would be different, because of different internal dependencies used or the platform dependent code written.

So this raises a question, as a high level crate, shouldn't the top level transport errors be predefined?

So for example, this is the current ClientInitialzieError enum:

pub enum ClientInitializeError<E> {
    #[error("expect initialized response, but received: {0:?}")]
    ExpectedInitResponse(Option<ServerJsonRpcMessage>),

    #[error("expect initialized result, but received: {0:?}")]
    ExpectedInitResult(Option<ServerResult>),

    #[error("conflict initialized response id: expected {0}, got {1}")]
    ConflictInitResponseId(RequestId, RequestId),

    #[error("connection closed: {0}")]
    ConnectionClosed(String),

    #[error("Send message error {error}, when {context}")]
    TransportError {
        error: E,
        context: Cow<'static, str>,
    },

    #[error("Cancelled")]
    Cancelled,
}

So majority of variants are defined fine, but in TransportError we are taking a generic error, which might be platform/dependency dependent.

But ideally we should not rely on third party dependencies for some code that we are providing. Like for SSE init error, we throw ClientInitializeError<SseTransportError<reqwest::Error>>

But what if reqwest crate is updated, and now they don't have reqwest::Error anymore, and I am building something which needs the latest version of reqwest but rmcp is still on previous version of the dependency, so there would be no way for me to build my app using reqwest and rmcp together.

So in my opinion, rmcp as a crate should have a domain defined, a domain is where all the high level Errors, traits, structs and orchestration logic is defined. Orchestration logic as in, the code flow is already defined via traits, so you just use the traits (preferably not dynamically dispatched) in the domain, and the platform/dependency specific implementation is moved to a new project/workspace.

Proposal:

crates/
|- rmcp-core # this would be the domain with all traits, errors, and orchestration logic is defined, and it's independent.
|- rmcp-sse # SSE impl
|- rmcp-stdio # stdio impl
|- rmcp-straming-http # Streaming HTTP impl
|- rmcp # Here we use the SSE/Stdio/Streaming HTTP implementations and expose it to public.   

Advantages:

• This way rmcp will only expose the platform/dependency neutral code from the domain, and internally use the platform/dependency code which public do not need to care about.
• This will make codebase much more maintainable and scalable, it will be very easy to change underlying implementations without making any breaking changes and in future you will easily be able to add/remove transports without making much changes at API level.

Edit 1:

For errors, we can have nested enums like:

Leaf level error enum:

pub enum ClientInitError {
  SseError(...),
  StdioError(...),
 ...
}

Parent level:

pub enum Error {
  ClientInitError(ClientInitError)
 ...
}

[4t145]

Proposal:

We have considered about it. But finally decide to maintain those official supported transport type in just one rmcp crate. Maybe we can seperate them some day but not in 0.2.x. This could be a big breaking change I guess.

So I want to paste a more specific code to make my plan more clear. We can make InitializeError independent of the specific transport error type, and you can downcast it to a specific one.

#[derive(Debug, thiserror::Error)]
pub enum ServiceRunningError {
    #[error("Service error: {0}")]
    Service(#[from] ServiceError),
    #[cfg(feature = "client")]
    #[error("Client initialization error: {0}")]
    // No more generic error type here!
    ClientInitialize(#[from] ClientInitializeError),
    #[cfg(feature = "server")]
    #[error("Server initialization error: {0}")]
    ServerInitialize(#[from] ServerInitializeError),
    #[error("Runtime error: {0}")]
    Runtime(#[from] tokio::task::JoinError),
    // and cancellation shouldn't be an error?
}

// Or we can just name it TransportError
#[derive(Debug, thiserror::Error)]
#[error("Transport [{transport}] error: {error}")]
pub struct DynamicTransportError {
    pub transport: Cow<'static, str>,
    #[source]
    pub error: Box<dyn std::error::Error + Send + Sync>,
}

impl DynamicTransportError {
    pub fn new<T: Transport<R>, R: ServiceRole>(e: T::Error) -> Self {
        Self {
            transport: T::name(),
            error: Box::new(e),
        }
    }
    pub fn downcast<T: Transport<R>, R: ServiceRole>(self) -> Result<T::Error, Self> {
        if !self.error.is::<T::Error>() || self.transport != T::name() {
            return Err(self);
        } else {
            Ok(self
                .error
                .downcast::<T::Error>()
                .map(|e| *e)
                .expect("type is checked"))
        }
    }
    pub fn is<T: Transport<R>, R: ServiceRole>(&self) -> bool {
        self.error.is::<T::Error>() && self.transport == T::name()
    }
}

And also, replace the Box<dyn Error> in ServerError with newly introduced DynamicTransportError.

And then, you can just throw any error from a call on type RunnningService as a ServiceRunningError. This would hide the underlying error type until you want to downcast it, which should still satisfy your requirement from what I understand.

[4t145]

I made a pr on this, please check it #308 .

[ssddOnTop]

According to the plan that you've proposed above I have some feedbacks:

This could be a big breaking change I guess.
Separating workspace does not mean having separate crates, there will be single crate rmcp which just exposes the inner crates.

2. My proposal was that we should not need ANY other dependency within the error type (or anything that will be exposed to public) so ideally we should not need DynamicTransportError at all. But overall this seems fine and it would work for my use case at least.

PS:
But I'd suggest not to close this issue just after #308 and we should work on avoiding exposing any third party dependencies' errors. DynamicTransportError will definitely work for majority of the use cases, but at the same time it's safe to assume that no user will downcast the errors since the user won't know what to downcast it to.

[4t145]

since the user won't know what to downcast it to

I think if user use rmcp as a directly dependency, they should know (and also should be able to control) the transport types they are using so they can downcast the error.

But I'd suggest not to close this issue just after #308

I will remain this issue open.

As for the project structure, maybe we can open a new issue on this. There was some one suggested something like this in #95. We should consider them together.