feat(server)!: convert server.rs internals + on_disconnected to ServerError#1244
Open
Greg Lamberson (glamberson) wants to merge 5 commits into
Open
Conversation
4 tasks
Greg Lamberson (glamberson)
force-pushed
the
feat/server-typed-error-server
branch
from
8cd5e23 to
e5c33c7
Compare
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.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-typed-error-server
branch
from
e5c33c7 to
b368125
Compare
This was referenced May 20, 2026
Greg Lamberson (glamberson)
force-pushed
the
feat/server-typed-error-server
branch
from
b368125 to
b098ace
Compare
4 tasks
Greg Lamberson (glamberson)
force-pushed
the
feat/server-typed-error-server
branch
from
b098ace to
a0b1000
Compare
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
…rverError Second step of the staged migration started in the previous commit. Replaces anyhow construction sites in modules whose internal flow does not pass through the ConnectionHandler::on_disconnected callback (which still takes &anyhow::Error and is the subject of a separate follow-up PR per Devolutions#1209): - encoder/mod.rs: ~15 anyhow! / .context() / bail! sites converted to typed ServerError variants (Encode, Reason, Custom). EncodeError sources go through ServerError::encode (matching ConnectorErrorExt::encode). spawn_blocking JoinError, qoi codec errors, and zstd error codes go through ServerError::custom or ServerError::reason as appropriate. - helper.rs: TLS cert/key loading paths construct ServerError::io for std::io::Error sources, ServerError::reason for missing-key cases, ServerError::custom for x509-cert and PEM parsing errors. Removes the from_anyhow bridge and the inner-fn split introduced in the previous commit. - echo.rs: build_echo_request returns ServerResult, builds errors via ServerError::custom directly. send_request keeps its already- typed Channel and Reason variants. server.rs internals stay on anyhow because they propagate into ConnectionHandler::on_disconnected which still takes &anyhow::Error; that conversion plus the remaining server.rs internal sites land in PR Devolutions#3. The anyhow dep stays in Cargo.toml for now and will be dropped when the public traits (ConnectionHandler, RdpServerDisplay, and RdpServerDisplayUpdates) finish their typed migration.
Replaces the closure-based `.map_err(|e| ServerError::custom(...))?` pattern with the `ServerResultExt::with_context` ext-trait introduced in Devolutions#1242. Preserves the inner `Reason` kind from QoizHandler::new (was being lost programmatically inside a Custom-wrapping outer error) while replacing the context string with the more informative "failed to initialize qoiz handler" wording. Precheck-driven cleanup; no behavioral change beyond preserving the typed kind at this site.
…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
…sites Replaces the closure-based set_context pattern at the UpdateEncoder::new and client_loop call sites in run_connection with the ServerResultExt::with_context ext-trait. Same semantics (preserves the inner kind, replaces the context string), substantially less verbose. Precheck-driven cleanup; no behavioral change.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-typed-error-server
branch
from
a0b1000 to
08f744d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third step of the staged migration started in #1242 and continued in #1243. Combines the
on_disconnectedsignature change with theserver.rsinternal site conversion since both touch anyhow-flowing code; doing them together avoids a temporaryanyhow ↔ ServerErrortwo-way bridge during the intermediate state.Public API change
fn on_disconnected( &mut self, peer: SocketAddr, duration: Duration, - error: Option<&anyhow::Error>, + error: Option<&ServerError>, ) -> PostConnectionAction { ... }This is a breaking change for handler implementations of
ConnectionHandler. Pre-1.0, and per Marc-Andre Lureau (@elmarco)'s note on #1209: "I am ok with breaking API at this point :)".Internal changes
run_inner/run_connection_innerwrapper functions introduced in feat(server)!: introduce typed ServerError on the public API boundary #1242 are folded back intorun/run_connection. The accept loop calls the public method directly;result.as_ref().err()now feeds the newServerError-typed parameter naturally..context()/bail!sites inrun,run_connection,accept_finalize,handle_io_channel_data,handle_x224,handle_input_backlog, and theencode_share_data_pdu/deactivate_allhelpers replaced with typedServerErrorvariants. Pattern alignment withConnectorErrorExt:EncodeErrorsources →ServerError::encodeDecodeErrorsources →ServerError::decodestd::io::Errorsources →ServerError::ioOption<channel>with.ok_or_else→ServerError::channelbail!("Fastpath output not supported!")→ServerError::unsupportedServerError::customwith a static context.What this PR does NOT touch
RdpServerDisplay::updates()andRdpServerDisplayUpdates::next_update()trait methods still returnanyhow::Result. Their conversion is feat(server)!: convert display traits to ServerResult, drop anyhow dep #1245, which also drops theanyhowdependency entirely.from_anyhowprivate helper introduced in feat(server)!: introduce typed ServerError on the public API boundary #1242 is retained only at one boundary: thedisplay.updates()call site inrun_connectionwraps its anyhow result viafrom_anyhowuntil feat(server)!: convert display traits to ServerResult, drop anyhow dep #1245 lands.Stack ordering
This is step 3 of 4. The full chain (must merge in PR-number order):
ServerError/ServerErrorKind/ ext traits, convert 5 public functions, internal anyhow stays via private bridgeon_disconnectedparameter (breaking)Branch is stacked on
feat/server-typed-error-internal(#1243), which is stacked onfeat/server-typed-error(#1242).Coordinate with #1239
The
ConnectionHandlertrait was extended in #1239 (SuppressOutput / RefreshRectangle / FrameAcknowledge handlers). If this PR lands first, #1239 needs a trivial rebase to the new trait shape (the new methods stay; onlyon_disconnected's signature changes around them). If #1239 lands first, this PR rebases onto its trait shape. Either order works.Test plan
cargo xtask check fmt -vcleancargo xtask check lints -vclean (workspace, all-targets, with helper + __bench features)cargo xtask check tests -vpassescargo build --workspace --all-targetscleanTracking: #1209.