Repository navigation
Conversation
`DrdynvcClient::with_dynamic_channel` registers a one-shot listener: once the channel has been created, a second DYNVC_CREATE_REQ for the same name is answered with NO_LISTENER. Servers do re-create channels mid-session: xrdp re-creates the graphics pipeline channel after every resize-triggered Deactivation-Reactivation Sequence, so the client silently loses EGFX and the display stays at the previous size. Add `DrdynvcClient::with_dynamic_channel_factory`, which builds a fresh processor for every create request while keeping typed lookup through `get_dvc`, resolved to the most recently created instance. Use it for the display control and graphics pipeline channels of ironrdp-client. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
kind/protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2050 adds DrdynvcClient::with_dynamic_channel_factory (FactoryListener + DynamicChannelSet::register_factory) so servers can re-create pre-registered DVCs (xrdp re-creates EGFX after a Deactivation-Reactivation Sequence), and migrates DisplayControl and GraphicsPipeline in the client to it while with_dynamic_channel stays one-shot. Independently verified against the head: the mechanism is protocol-conformant and the typed-lookup-follows-latest claim holds (try_create_channel overwrites type_id_to_channel_id). Two valid concerns survive scrutiny: (1) a re-create under a new channel ID never drops the previous active_channels entry — cleanup exists only via DYNVC_CLOSE or same-ID replacement — so each reactivation leaks the old processor, its CompleteData, its Drop-side close(), and any tunnel binding; (2) Echo and Rdpei registered one line below the new comment keep the exact NO_LISTENER failure mode the PR fixes, with no stated justification. Two valid low-severity compressions: F…
- [skeptical] Same-builder channels Echo and Rdpei keep the exact failure mode the PR fixes — low 🟡 ❓ — crates/ironrdp-client/src/rdp.rs
The PR's premise is that servers may re-create pre-registered DVCs during a session, yet in the same builder statement (rdp.rs:1589-1595) only DisplayControl and GraphicsPipeline move to with_dynamic_channel_factory; EchoClient, RdpeiClient, and the optional LocationClient/vmconnect/pipe-proxy registrations remain behind OnceListener and will answer a second DYNVC_CREATE_REQ with NO_LISTENER, silently losing those channels for the session. If the triggering condition is generic, the fix is incomplete and the same bug persists one line below the new comment; if it is xrdp/egfx-specific, the PR should state why the others are exempt (factory-izing stateful processors like RdpeiClient is not automatically safe). No evidence in the PR establishes whether observed servers re-create echo/rdpei, hence the question flag.
| /// Re-armable pre-registered DVC: a fresh processor is built for every DYNVC_CREATE_REQ. | ||
| /// | ||
| /// Servers may create the same dynamic channel again during a session (xrdp re-creates the | ||
| /// graphics pipeline channel after a Deactivation-Reactivation Sequence, for instance). A | ||
| /// [`OnceListener`] rejects the second request with NO_LISTENER, silently losing the channel. | ||
| struct FactoryListener<T, F> { | ||
| name: String, | ||
| first: Option<T>, | ||
| factory: F, | ||
| } | ||
|
|
||
| impl<T, F> DvcChannelListener for FactoryListener<T, F> | ||
| where | ||
| T: DvcClientProcessor + 'static, | ||
| F: FnMut() -> T + Send, | ||
| { | ||
| fn channel_name(&self) -> &str { | ||
| &self.name | ||
| } | ||
|
|
||
| fn create(&mut self, _channel_id: DynamicChannelId) -> Option<Box<dyn DvcClientProcessor>> { | ||
| let processor = match self.first.take() { | ||
| Some(processor) => processor, | ||
| None => (self.factory)(), | ||
| }; | ||
| Some(Box::new(processor)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] Re-creating a factory channel under a new channel ID never drops the previous instance — medium 🟠 ❓ — FactoryListener::create unconditionally yields a fresh processor, and try_create_channel (client.rs:711-734) inserts it under the new channel ID and updates type_id_to_channel_id, but nothing removes the older active_channels entry for the same channel name; cleanup only happens on an explicit DYNVC_CLOSE (close_channel -> remove_by_channel_id) or same-ID replacement. The PR's own test models channel 3 -> 9 with no Close in between, so each server-driven re-creation (e.g. after a Deactivation-Reactivation Sequence) leaves the previous DynamicVirtualChannel, its CompleteData, its Drop-triggered processor.close(), and any tunnel_channels binding alive for the rest of the session: unbounded growth across repeated resizes, duplicated entries in Debug/values() iteration, and stale routing if soft-sync was involved. Whether real servers (xrdp) close the old ID before re-creating — which would mask the leak — is not established by the evidence, hence the question flag; the gap exists in the feature as designed. Consider dropping or closing the prior active-channel for the same name (or its type_id) when a factory re-create succeeds.
| /// Re-armable pre-registered DVC: a fresh processor is built for every DYNVC_CREATE_REQ. | ||
| /// | ||
| /// Servers may create the same dynamic channel again during a session (xrdp re-creates the | ||
| /// graphics pipeline channel after a Deactivation-Reactivation Sequence, for instance). A | ||
| /// [`OnceListener`] rejects the second request with NO_LISTENER, silently losing the channel. | ||
| struct FactoryListener<T, F> { | ||
| name: String, | ||
| first: Option<T>, | ||
| factory: F, | ||
| } | ||
|
|
||
| impl<T, F> DvcChannelListener for FactoryListener<T, F> | ||
| where | ||
| T: DvcClientProcessor + 'static, | ||
| F: FnMut() -> T + Send, | ||
| { | ||
| fn channel_name(&self) -> &str { | ||
| &self.name | ||
| } | ||
|
|
||
| fn create(&mut self, _channel_id: DynamicChannelId) -> Option<Box<dyn DvcClientProcessor>> { | ||
| let processor = match self.first.take() { | ||
| Some(processor) => processor, | ||
| None => (self.factory)(), | ||
| }; | ||
| Some(Box::new(processor)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] FactoryListener's first slot can be dropped in favor of always calling the factory — low 🟡 — register_factory already calls the factory to learn the channel name, so the first: Option<T> field and the match/take in create() exist only to hand the registration-time instance to the first DYNVC_CREATE_REQ. The struct can be reduced to { name, factory } with create() unconditionally returning Box::new((self.factory)()), and register_factory discarding the probe instance after reading channel_name. Observable behavior is unchanged for pure factories, which covers every call site in this PR (DisplayControlClient::new, GraphicsPipelineClient::new with unit EgfxHandler, RecordedDvc::default) and the new tests; the only tradeoff is for hypothetical factories with mutable captured state, and since the API is introduced here, documenting 'the factory is invoked per create' in the existing doc comment is a fair contract.
| fn register_factory<T, F>(&mut self, mut factory: F) | ||
| where | ||
| T: DvcClientProcessor + 'static, | ||
| F: FnMut() -> T + Send + 'static, | ||
| { | ||
| let first = factory(); | ||
| let name = first.channel_name().to_owned(); | ||
| self.listeners.insert( | ||
| name.clone(), | ||
| ListenerEntry { | ||
| listener: Box::new(FactoryListener { | ||
| name, | ||
| first: Some(first), | ||
| factory, | ||
| }), | ||
| type_id: Some(TypeId::of::<T>()), | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[code-compressor] Third copy of the ListenerEntry insert boilerplate in DynamicChannelSet — low 🟡 — register_factory duplicates the same ~9-line listeners.insert(ListenerEntry { listener: Box::new(..), type_id: .. }) block already present in register_listener (client.rs:654-663) and register_once (665-674). A single private helper (e.g. fn insert_listener(&mut self, listener: impl DvcChannelListener + 'static, type_id: Option<TypeId>) that derives the map key via listener.channel_name().to_owned() after boxing) collapses all three call sites to 2-3 lines each. Behavior is identical: OnceListener and FactoryListener both implement channel_name() independently of the boxed state, so the name derived from the boxed listener matches what the current code computes before boxing. This keeps future listener kinds from adding a fourth copy.
github-actions
Bot
added
ai-reviewed/1
This branch was successfully deployed
1 participant
Problem
DrdynvcClient::with_dynamic_channelregisters the processor behind a one-shot listener (OnceListener). After the channel has been created once, any furtherDYNVC_CREATE_REQfor the same name is answered withNO_LISTENER.Servers do re-create dynamic channels during a session. Observed with xrdp 0.10.1 and the graphics pipeline: when the client resizes through display control, xrdp performs a Deactivation-Reactivation Sequence ("Core reset done") and then creates
Microsoft::Windows::RDS::Graphicsagain (xrdp_egfx_createshows up a second time in the server log). The client refuses the second request, so EGFX is lost for the rest of the session and the output stays at the previous size.Fix
ironrdp-dvc: newDrdynvcClient::with_dynamic_channel_factory(|| ...). A factory listener builds a fresh processor for every create request. Typed lookup throughget_dvc::<T>()keeps working, and resolves to the most recently created instance becausetry_create_channelalready updatestype_id_to_channel_id.ironrdp-client: the display control and graphics pipeline channels are registered through the factory.with_dynamic_channelkeeps its existing behavior; nothing changes for other callers.Tests
In
crates/ironrdp-testsuite-core/tests/dvc/client.rs:one_shot_dynamic_channel_rejects_a_second_create_requestpins down the existingwith_dynamic_channelbehavior.factory_dynamic_channel_is_recreated_and_typed_lookup_follows_the_latest_instancecovers the new registration: both creates succeed andget_dvcfollows the second instance.Run locally (Linux container, toolchain 1.94.1):
cargo xtask check fmt,cargo xtask check lints,cargo xtask check typos,cargo xtask check tests.End to end, the same change is applied to
ironrdp-webwith the graphics pipeline enabled (not part of this PR). Against xrdp 0.10.1, repeated grow/shrink resizes now re-negotiate EGFX every time (xrdp_egfx_reset_graphicswith the new size), and keyboard input still works afterwards.🤖 Generated with Claude Code