Skip to content

feat: split ironrdp-client into a reusable library + ironrdp-viewer binary#1309

Merged
Benoît Cortier (CBenoit) merged 6 commits into
masterfrom
copilot/propose-ironrdp-separation
May 26, 2026
Merged

feat: split ironrdp-client into a reusable library + ironrdp-viewer binary#1309
Benoît Cortier (CBenoit) merged 6 commits into
masterfrom
copilot/propose-ironrdp-separation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 25, 2026

  • Change ClipboardType in ironrdp-client library to Enable, Disable, Stub (remove platform-specific variants)
  • Update ClipboardType in ironrdp-viewer to match: Enable, Disable, Stub (remove Default and platform-specific variants)
  • Update all usages of ClipboardType in viewer's main.rs and config.rs
  • Copy old ironrdp-client/README.md to ironrdp-viewer/README.md
  • Update ironrdp-client/README.md (fix stale UnboundedSender reference)
  • Update test assertion in ironrdp-testsuite-extra
  • Add parse_config_from helper and use it in tests
  • Run cargo fmt

Copilot AI changed the title refactor: split ironrdp-client into library + ironrdp-viewer binary (PR A) Split ironrdp-client into a reusable library + ironrdp-viewer binary (PR A) May 25, 2026
@glamberson
Copy link
Copy Markdown
Contributor

Quick notes while PR A is still draft.

  1. The RdpOutputEventSender trait drop is the cleaner shape. Our 2026-05-20 review on Add agentic RDP CLI and localhost CI workflow #1289 praised the trait specifically as the mechanism that decoupled RdpClient from winit::EventLoopProxy. Reading PR A: the plain tokio::sync::mpsc::UnboundedSender<RdpOutputEvent> solves the same decoupling problem more directly. No vtable per event, no Box<dyn> allocation, no winit in the library's dep graph, and the embedder's bridging task (the rt.spawn block in viewer/src/main.rs) is short and obvious. Endorse the swap. The mpsc is the cleaner mechanism for what our prior review was actually trying to praise.

  2. The unbounded-channel concern from our 2026-05-20 review on Add agentic RDP CLI and localhost CI workflow #1289 survives the reshape. That review flagged "memory grows without bound on chatty server with the unbounded shape; snapshot-only-keeps-latest model wants bounded + try_send + drop-oldest." PR A still uses mpsc::unbounded_channel::<RdpOutputEvent>() in viewer/src/main.rs with mpsc::UnboundedSender<RdpOutputEvent> on RdpClient. The architectural shape is right for PR A's "pure mechanical split" framing; bounded + drop-oldest semantics for the output channel feels like the natural fit for PR C (alongside the per-channel Cargo features and runtime gates already scoped there). Flagging here so it does not slip on the PR-A to PR-B to PR-C handoff.

  3. Cross-reference for ironrdp-web becomes unresponsive when remote framebuffer updated rapidly #790: PR A establishes the library + facade pattern (ironrdp-client library + ironrdp-viewer facade) in tree. The shape we proposed at ironrdp-web becomes unresponsive when remote framebuffer updated rapidly #790 (comment) (ironrdp-wasm pure-Rust library + ironrdp-web facade + new lamco facade) is structurally the same precedent applied to the wasm track. With PR A in flight, the precedent is established. Happy to move the ironrdp-web becomes unresponsive when remote framebuffer updated rapidly #790 discussion forward to a concrete file/module layout proposal whenever you have cycles. No urgency.

Looks good from my perspective. The library + facade split is the structurally clean shape for an embeddable RDP engine, and reading the diff confirms the split is honest: winit, clap, inquire, propertyset, rdpfile, and cfg are all out of the library's dep graph, tokio is narrowed from full to what the library actually uses, and the bridging task in the binary is short enough to be self-evident.

@CBenoit
Copy link
Copy Markdown
Member

Copilot Rebase on top of master.

Also, use a bounded mpsc channel with backpressure. Either wait for important messages, or drop irrelevant ones. Ideally, do not drop unless absolutely necessary.

Comment thread crates/ironrdp-client/src/config.rs
Comment thread crates/ironrdp-viewer/src/config.rs
Comment thread crates/ironrdp-client/README.md
Copilot AI force-pushed the copilot/propose-ironrdp-separation branch from 2732d16 to da34f7f Compare May 26, 2026 12:27
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 26, 2026

Copilot Rebase on top of master.

Also, use a bounded mpsc channel with backpressure. Either wait for important messages, or drop irrelevant ones. Ideally, do not drop unless absolu...

Rebased on master (resolved conflict in ironrdp-client/src/config.rs, ported the new --desktop-width/--desktop-height CLI args to ironrdp-viewer).

Switched the output event channel from unbounded to bounded (mpsc::channel(64)) with async backpressure: all output_event_sender.send(…) calls now .await, so the RDP session naturally slows down when the viewer can't keep up. No messages are dropped — important events like ConnectionFailure and Terminated are always delivered, and display events apply backpressure rather than being lost. Commit: da34f7f.

@CBenoit Benoît Cortier (CBenoit) changed the title Split ironrdp-client into a reusable library + ironrdp-viewer binary (PR A) feat: split ironrdp-client into a reusable library + ironrdp-viewer binary May 26, 2026
Copilot stopped work on behalf of Benoît Cortier (CBenoit) due to an error May 26, 2026 12:46
Copilot AI changed the title feat: split ironrdp-client into a reusable library + ironrdp-viewer binary Simplify ClipboardType to Enable/Disable/Stub and restore viewer README May 26, 2026
@CBenoit Benoît Cortier (CBenoit) changed the title Simplify ClipboardType to Enable/Disable/Stub and restore viewer README feat: split ironrdp-client into a reusable library + ironrdp-viewer binary May 26, 2026
Comment thread crates/ironrdp-testsuite-extra/tests/config_rdp.rs Outdated
@CBenoit
Copy link
Copy Markdown
Member

Copilot Finally, run cargo fmt

@CBenoit Benoît Cortier (CBenoit) marked this pull request as ready for review May 26, 2026 15:17
@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented May 26, 2026

Greg Lamberson (@glamberson) I removed the unbounded mpsc channel in favor of a bounded one. However, we’re not dropping anything, instead we introduce backpressure by awaiting until there is space again in the channel. I’m open to the idea of "snapshot-only-keeps-latest model wants bounded + try_send + drop-oldest", but we need to be careful to ensure the "latest snapshot" properly reflects the correct state even if we applied everything else. Since this is more elaborate, I’m leaving this improvement for a future PR, and I welcome your contribution if you already have a good idea on how to do that well. For now, backpressure should give a hint to the chatty server that the client is unable to follow (if the server is smart enough to adjust).

@CBenoit Benoît Cortier (CBenoit) merged commit 361bdc2 into master May 26, 2026
20 of 21 checks passed
@CBenoit Benoît Cortier (CBenoit) deleted the copilot/propose-ironrdp-separation branch May 26, 2026 16:15
@glamberson
Copy link
Copy Markdown
Contributor

Benoît Cortier (@CBenoit) Thanks for the direct address; appreciated. Bounded + backpressure is a solid resolution on the immediate concern, and merging the PR keeps the #1289 reshape moving.

On the drop-oldest invitation: agreed it requires more care than a straightforward swap. The "latest snapshot properly reflects correct state" warning is the load-bearing constraint, particularly for framebuffer region updates which are stateful, not snapshot-style. A correct shape probably needs per-variant DropPolicy classification (something like MustDeliver / Coalesce / DropOldest) with the Coalesce policy maintaining a dirty-region accumulator on the receiver side so framebuffer updates can be merged without losing region context.

Filing as deferred-future-work on our side; tracking in our internal backlog. We will come back with a design issue rather than a code PR once we have a shape worth proposing, and after measuring real-traffic baseline against the current bounded + backpressure model to confirm there is a tail-latency benefit worth the implementation cost. No timeline commitment.

@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented May 27, 2026

Greg Lamberson (@glamberson) The general idea makes perfect sense to me. Thank you.

and after measuring real-traffic baseline against the current bounded + backpressure model to confirm there is a tail-latency benefit worth the implementation cost

Fully agreed. Let us know if you find out that it’s worth implementing, and looking forward to the proposal if that is the case!

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.

4 participants