Skip to content

feat(server)!: introduce typed ServerError on the public API boundary#1242

Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/server-typed-error
Open

feat(server)!: introduce typed ServerError on the public API boundary#1242
Greg Lamberson (glamberson) wants to merge 1 commit into
Devolutions:masterfrom
lamco-admin:feat/server-typed-error

Conversation

@glamberson
Copy link
Copy Markdown
Contributor

Summary

First in a staged migration toward a typed public error story for ironrdp-server, addressing #1209. Introduces:

pub type ServerError = ironrdp_error::Error<ServerErrorKind>;
pub type ServerResult<T> = Result<T, ServerError>;

ServerErrorKind is a #[non_exhaustive] enum with concretely typed variants (Encode, Decode, Io, Channel, Unsupported, Reason, General, Custom). Sources are attached through ironrdp_error::Error::with_source rather than embedded as Box<dyn Error> in variant data. This is byte-for-byte the shape of ConnectorErrorKind in ironrdp-connector, so the connection-management layer stays internally consistent.

Why this shape

The audit before drafting found that ironrdp-connector, ironrdp-session, ironrdp-pdu, ironrdp-mstsgu, and ironrdp-core (EncodeError/DecodeError) all use the same ironrdp_error::Error<Kind> pattern. The thiserror-based bare enums elsewhere in the workspace (GccError, BulkError, etc.) are leaf-layer errors at the PDU/codec level; the connection-management layer sister crate to ironrdp-server is the right template.

Scope of this PR

Public function signatures only:

  • RdpServer::runServerResult<()>
  • RdpServer::run_connection<S>ServerResult<()>
  • TlsIdentityCtx::init_from_pathsServerResult<Self>
  • TlsIdentityCtx::make_acceptorServerResult<TlsAcceptor>
  • EchoServerHandle::send_requestServerResult<()>

Internal call sites continue to use anyhow::Result. A private from_anyhow helper bridges at the public boundary. The ConnectionHandler::on_disconnected(error: Option<&anyhow::Error>) parameter is unchanged in this PR.

Companion ext traits

pub trait ServerErrorExt {
    fn encode(error: EncodeError) -> Self;
    fn decode(error: DecodeError) -> Self;
    fn io(context: &'static str, error: io::Error) -> Self;
    fn channel(context: &'static str) -> Self;
    fn unsupported(context: &'static str) -> Self;
    fn general(context: &'static str) -> Self;
    fn reason(context: &'static str, reason: impl Into<String>) -> Self;
    fn custom<E>(context: &'static str, error: E) -> Self
    where E: core::error::Error + Sync + Send + 'static;
}
pub trait ServerResultExt {
    fn with_context(self, context: &'static str) -> Self;
    fn with_source<E>(self, source: E) -> Self
    where E: core::error::Error + Sync + Send + 'static;
}

Mirrors ConnectorErrorExt / ConnectorResultExt exactly.

Staged follow-ups (separate PRs)

  • PR Add fuzzers #2: Convert internal ? chains and .context() calls in ironrdp-server to use the typed kinds directly. Remove the anyhow dependency when the last anyhow! is gone. Touches encoder/mod.rs (heaviest), server.rs, display.rs, builder.rs.
  • PR Panic in UserDataHeader::from_buffer due to integer overflow #3: Change ConnectionHandler::on_disconnected(error: Option<&anyhow::Error>) to Option<&ServerError>. Standalone breaking change for handler implementations.

Breaking change

Marked with ! in the conventional commit. Consumers of the five listed public functions need to update their Result types. Pre-1.0, and per Marc-Andre Lureau (@elmarco)'s note on #1209: "I am ok with breaking API at this point :)".

Test plan

  • cargo xtask check fmt -v clean
  • cargo xtask check lints -v clean (workspace, all-targets, with helper + __bench features)
  • cargo xtask check tests -v passes
  • cargo build --workspace --all-targets clean (one example consumer in crates/ironrdp/examples/server.rs updated to convert at its own boundary)

Closes part of #1209.

Introduces a typed error story for ironrdp-server, mirroring the shape
already used by ironrdp-connector and the rest of the connection-management
layer:

  pub type ServerError = ironrdp_error::Error<ServerErrorKind>;
  pub type ServerResult<T> = Result<T, ServerError>;

ServerErrorKind is a non-exhaustive enum with concretely typed variants
for the failure modes the crate surfaces (Encode, Decode, Io, Channel,
Unsupported, Reason, General, Custom). Sources are attached through
ironrdp_error::Error::with_source rather than embedded as Box<dyn Error>
in variant data, matching ConnectorErrorKind exactly.

This first PR converts public boundaries only:

  - RdpServer::run                -> ServerResult<()>
  - RdpServer::run_connection<S>  -> ServerResult<()>
  - TlsIdentityCtx::init_from_paths -> ServerResult<Self>
  - TlsIdentityCtx::make_acceptor   -> ServerResult<TlsAcceptor>
  - EchoServerHandle::send_request  -> ServerResult<()>

Internal call sites continue to use anyhow::Result during the staged
migration. A private  helper bridges at the public boundary.
A follow-up PR converts internal sites to use the typed kinds directly
and drops the anyhow dependency. A second follow-up changes the
ConnectionHandler::on_disconnected error parameter from
Option<&anyhow::Error> to Option<&ServerError>.

This is a breaking change to consumers of the listed public functions:
return types change from anyhow::Result<T> to ServerResult<T>.

Tracking: Devolutions#1209
Greg Lamberson (glamberson) pushed a commit to lamco-admin/IronRDP that referenced this pull request May 13, 2026
…rError

Third step of the staged migration started in Devolutions#1242 and continued in
the previous commit. Combines the on_disconnected signature change with
the server.rs internal site conversion since both touch anyhow-flowing
code; doing them together avoids a temporary anyhow-to-ServerError
two-way bridge during the intermediate state.

Public API change:

  ConnectionHandler::on_disconnected
      error: Option<&anyhow::Error>  ->  Option<&ServerError>

This is a breaking change for handler implementations.

Internal changes:

  - The two run_inner / run_connection_inner wrapper functions are
    folded back into run / run_connection. The accept loop calls the
    public method directly; result.as_ref().err() now feeds the new
    ServerError-typed parameter naturally.
  - ~25 .context() / bail! sites in run, run_connection, accept_finalize,
    handle_io_channel_data, handle_x224, handle_input_backlog, and the
    encode_share_data_pdu / deactivate_all helpers replaced with typed
    ServerError variants. Pattern alignment with ConnectorErrorExt:
    EncodeError sources -> ServerError::encode, DecodeError sources ->
    ServerError::decode, std::io::Error sources -> ServerError::io,
    Option<channel> with .ok_or_else -> ServerError::channel,
    bail!("Fastpath output not supported!") ->
    ServerError::unsupported, anything else -> ServerError::custom with
    a static context.
  - The from_anyhow bridge is retained only at one boundary: the
    RdpServerDisplay::updates() trait method still returns
    anyhow::Result, so the call site in run_connection wraps the anyhow
    result via from_anyhow. PR Devolutions#4 of this migration converts the
    display traits and drops the bridge entirely.

The anyhow dependency stays in Cargo.toml because RdpServerDisplay,
RdpServerDisplayUpdates, and a small handful of consumer-facing trait
returns still use anyhow::Result. PR Devolutions#4 finishes that conversion and
drops the dep.

Tracking: Devolutions#1209
Greg Lamberson (glamberson) pushed a commit to lamco-admin/IronRDP that referenced this pull request May 13, 2026
Final step of the staged migration started in Devolutions#1242, continued in
Devolutions#1243 and Devolutions#1244. Completes the typed error story for ironrdp-server.

Public API changes:

  RdpServerDisplay::updates
      -> Result<Box<dyn RdpServerDisplayUpdates>, anyhow::Error>
      => ServerResult<Box<dyn RdpServerDisplayUpdates>>

  RdpServerDisplayUpdates::next_update
      -> Result<Option<DisplayUpdate>, anyhow::Error>
      => ServerResult<Option<DisplayUpdate>>

These are breaking changes for handler implementations of the two
display traits.

Internal changes:

  - The from_anyhow private bridge and the AnyhowError wrapper struct
    in error.rs are removed.
  - The anyhow dependency is removed from ironrdp-server/Cargo.toml.
  - builder.rs's NoopDisplayUpdates / NoopDisplay impls and the
    docstring example in display.rs and README.md are updated to match
    the new trait shapes.
  - The example in crates/ironrdp/examples/server.rs and the integration
    test in crates/ironrdp-testsuite-extra/tests/main.rs are updated to
    return ServerResult from their RdpServerDisplay/Updates impls.
  - benches/src/perfenc.rs is updated to construct ServerError variants
    instead of anyhow::Error and converts at its own anyhow::Result main
    boundary via .map_err(|e| anyhow::anyhow!(e)).

After this commit, ironrdp-server has no anyhow dependency and the
public surface is fully typed against ServerError. Closes Devolutions#1209.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants