feat(dvc): add DvcChannelListener for multi-instance DVC support by uchouT · Pull Request #1142 · Devolutions/IronRDP

uchouT (uchouT) · 2026-03-03T10:44:49Z

Resolves #1135

implement the client-side without breaking the current API signatures by introducing OnceListener wrapper for pre-registered DVCs.
implement server side with a new method create_channel for DrdynvcServer.

older discussion

I try to implement the client-side without breaking the current API signatures by introducing OnceListener wrapper for pre-registered DVCs. But get_dvc_by_type_id's behavior has changed, it returns None until CreateRequest arrives.

This is unavoidable — the old 1:1 name→channel mapping cannot support multiple channels with the same name. The ChannelId is assigned by the server at CreateRequest time, so the DVC cannot exist before that point. And this behavior follows the RDPEDYC's expectation.

Marc-Andre Lureau (@elmarco) Benoît Cortier (@CBenoit) Does this approach acceptable and make sense?

Marc-Andre Lureau (elmarco) · 2026-03-03T11:54:06Z

uchouT (@uchouT) lgtm, perhaps squash or reorganize some of your changes (PERF, ..)

uchouT (uchouT) · 2026-03-03T12:33:08Z

uchouT (uchouT (@uchouT)) lgtm, perhaps squash or reorganize some of your changes (PERF, ..)

Squashed, thanks for reminding.

I found that ironrdp-session would be affected by this, but I didn't go further.

IronRDP/crates/ironrdp-session/src/x224/mod.rs

Lines 98 to 100 in db2f40b

    
           pub fn get_dvc<T: DvcProcessor + 'static>(&self) -> Option<&DynamicVirtualChannel> { 
        
               self.get_svc_processor::<DrdynvcClient>()?.get_dvc_by_type_id::<T>() 
        
           }

If this is acceptable, I will try the server-side implementation.

Greg Lamberson (glamberson) · 2026-03-03T22:07:39Z

If this is acceptable, I will try the server-side implementation.

This is good work. The listener/factory pattern is exactly the right approach for multi-instance DVCs. The OnceListener wrapper preserving backward compatibility for single-instance channels is a nice touch.

We're using IronRDP on the server side (lamco-rdp-server) and this direction aligns well with what we'll need for URBDRC support down the road. The encode_dvc_messages() and DvcProcessor trait being untouched means our current code should be unaffected by the client-side refactor.

Looking forward to the server-side implementation with take_new_channels(). Happy to help test once that lands.

Regards,
Greg Lamberson
Lamco Development

uchouT (uchouT) · 2026-03-04T09:51:07Z

Looking forward to the server-side implementation with take_new_channels(). Happy to help test once that lands.

Thanks for the review, the positive feedback, and your willingness to help test, Greg Lamberson (@glamberson)!

Regarding the take_new_channels() approach, I still have some hesitations about the implementation details. I left my thoughts here #1135 (comment) , would love to hear your input.

Copilot

Pull request overview

This PR implements multi-instance DVC (Dynamic Virtual Channel) support by introducing a DvcChannelListener factory trait that produces a new DvcClientProcessor instance per DYNVC_CREATE_REQ. It preserves the existing single-instance API by internally wrapping pre-registered DvcProcessor instances in an OnceListener. On the server side, a new create_channel method is added to DrdynvcServer to allow opening new DVC channels mid-session.

Changes:

Introduces DvcChannelListener trait and OnceListener adapter, refactors DynamicChannelSet (moved from lib.rs to client.rs) to key active channels by DynamicChannelId rather than by name
Adds DrdynvcServer::create_channel() for mid-session server-side DVC channel creation
Removes the now-obsolete DynamicChannelSet struct from lib.rs, replacing DynamicVirtualChannel::new with from_boxed

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`crates/ironrdp-dvc/src/client.rs`	Adds `DvcChannelListener` trait + `OnceListener`, refactors `DynamicChannelSet` to support multi-instance channels, updates `process()` to use new listener-based creation flow
`crates/ironrdp-dvc/src/lib.rs`	Removes `DynamicChannelSet`, changes `DynamicVirtualChannel` constructor to `from_boxed`
`crates/ironrdp-dvc/src/server.rs`	Adds `create_channel()` for mid-session DVC channel creation; removes stale `FIXME` comment

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

crates/ironrdp-dvc/src/server.rs

crates/ironrdp-dvc/src/client.rs

uchouT (uchouT)

Addressed Copilot's review. Benoît Cortier (@CBenoit)

crates/ironrdp-dvc/src/client.rs

crates/ironrdp-dvc/src/server.rs

crates/ironrdp-dvc/src/client.rs

Signed-off-by: uchouT <i@uchout.moe>

Introduce DvcChannelListener trait and OnceListener to support same-name multi-instance dynamic virtual channels. DVCs are now created on-demand via try_create_channel() when the server sends a CreateRequest, rather than pre-allocated at registration. The existing with_dynamic_channel() API is preserved via OnceListener. BREAKING: get_dvc_by_type_id() now returns None until the server sends a CreateRequest for the channel. Previously it returned Some immediately after registration. Signed-off-by: uchouT <i@uchout.moe> perf(dvc): reduce another btreemap search Signed-off-by: uchouT <i@uchout.moe> perf(dvc): reduce needless allocation in OnceListener Signed-off-by: uchouT <i@uchout.moe>

Unlike `with_dynamic_channel`, this method is designed for runtime use and doesn't record a TypeId mapping. Signed-off-by: uchouT <i@uchout.moe> chore(dvc): docs modified and new fn attach_listener Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

uchouT (uchouT) · 2026-03-09T18:04:25Z

Benoît Cortier (@CBenoit) resolved merge conflicts and rebased.

Signed-off-by: uchouT <i@uchout.moe> chore(dvc): docs update, clarify limitation Signed-off-by: uchouT <i@uchout.moe> chore(dvc): clean style for edition 2024 Signed-off-by: uchouT <i@uchout.moe> chore: sytle fmt Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) · 2026-03-12T13:59:51Z

Sorry about the formatting issue! My local cargo fmt was unusable due to the transition from Rust edition 2021 to 2024, so I had to fix it manually.

It should be good now. Benoît Cortier (@CBenoit) , could you please re-trigger the CI when you have a chance? Thanks!

Benoît Cortier (CBenoit)

Great work! I think this is going in the right direction, thank you!

I’m being a little bit more picky because this is a fundamental building block for every dynamic virtual channel 🙂

crates/ironrdp-dvc/src/client.rs

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

crates/ironrdp-dvc/src/client.rs

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT)

Thanks for the detailed review. I've applied the suggestions and addressed the comments in the latest commits.

crates/ironrdp-dvc/src/client.rs

Benoît Cortier (CBenoit) · 2026-03-13T03:44:15Z

Are you sure this was a problem? It seems the compilation was successful

crates/ironrdp-dvc/src/client.rs

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) · 2026-03-13T06:58:24Z

Are you sure this was a problem? It seems the compilation was successful

Yes, the compilation was actually fine. I updated it mostly to keep the signature consistent with other methods. I removed the Send bound as you mentioned, and added the 'static bound (which Copilot also suggested, and it makes sense here).

Also, regarding this discussion, does my explanation make sense to you?

crates/ironrdp-dvc/src/client.rs

Signed-off-by: uchouT <i@uchout.moe>

Benoît Cortier (CBenoit)

At this point, this is looking good to me. Let’s address the last nits just because it’s a fundamental building blocks + add one last pass of Copilot (you don’t have to address the "useless" suggestions of course!) just to be sure, and merge 🚀

uchouT (uchouT) · 2026-03-13T07:43:59Z

At this point, this is looking good to me. Let’s address the last nits just because it’s a fundamental building blocks + add one last pass of Copilot (you don’t have to address the "useless" suggestions of course!) just to be sure, and merge 🚀

Nice to hear about that! I've updated. Please check, and if this is Ok I'll squash and rebase and ready for merge.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/ironrdp-dvc/src/client.rs:133

get_dvc_by_type_id calls TypeId::of::<T>(), which requires T: 'static, but the current bounds only require T: DvcProcessor. This won’t compile for non-'static processor types; add a + 'static bound (or T: 'static) to the method’s where clause to satisfy TypeId::of’s requirements.

    pub fn get_dvc_by_type_id<T>(&self) -> Option<&DynamicVirtualChannel>
    where
        T: DvcProcessor,
    {
        self.dynamic_channels.get_by_type_id(TypeId::of::<T>())
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

crates/ironrdp-dvc/src/client.rs

+    fn try_create_channel(
+        &mut self,
+        name: &DynamicChannelName,
+        channel_id: DynamicChannelId,
+    ) -> Option<&mut DynamicVirtualChannel> {
+        let entry = self.listeners.get_mut(name)?;
+        let processor = entry.listener.create()?;
+
+        if let Some(type_id) = entry.type_id {
+            self.type_id_to_channel_id.insert(type_id, channel_id);
+        }
+
+        let mut dvc = DynamicVirtualChannel::from_boxed(processor);
+        dvc.channel_id = Some(channel_id);
+        let dvc = match self.active_channels.entry(channel_id) {
+            alloc::collections::btree_map::Entry::Occupied(mut e) => {
+                e.insert(dvc);
+                e.into_mut()
+            }
+            alloc::collections::btree_map::Entry::Vacant(e) => e.insert(dvc),
+        };
+        Some(dvc)
+    }


Benoît Cortier (CBenoit) · 2026-03-13T08:33:36Z

I think the last two Copilot suggestions are valid. Do you think you could look into this before we merge, or you prefer to send a follow up PR?

uchouT (uchouT) · 2026-03-13T09:55:16Z

Agreed. I'd prefer to handle the integration tests in a follow-up PR, if that works for you. cc Greg Lamberson (@glamberson)

I believe the current state is safe to merge because the only potential panic risk would be calling OnceListener::channel_name() twice, but that instance is fully encapsulated and never exposed to users. ( DrdynvcClient doesn't have a get_listener API.

Besides, I noticed that DrdynvcClient silently overwrites the existing listener/channel if one is attached with the same name. The existing attach_dynamic_channel API actually shares this behavior.

IronRDP/crates/ironrdp-dvc/src/client.rs

Lines 65 to 70 in 9a1ac30

    
           pub fn attach_dynamic_channel<T>(&mut self, channel: T) 
        
           where 
        
               T: DvcProcessor + 'static, 
        
           { 
        
               self.dynamic_channels.insert(channel); 
        
           }

Should I add some documentation to clarify this before we merge?

Signed-off-by: uchouT <i@uchout.moe>

Benoît Cortier (CBenoit)

LGTM! Excellent work!

Benoît Cortier (CBenoit) · 2026-03-13T13:37:04Z

I believe the current state is safe to merge because the only potential panic risk would be calling OnceListener::channel_name() twice, but that instance is fully encapsulated and never exposed to users. ( DrdynvcClient doesn't have a get_listener API.

I’m not really worried about panics indeed. And we could wait for the RDPEUSB implementation to perform an integration test close to real-world usecases.

Should I add some documentation to clarify this before we merge?

I see you added it already. Thank you!

crates/ironrdp-dvc/src/client.rs

uchouT (uchouT) changed the title ~~Refactor/dvc~~ feat(dvc): add DvcChannelListener for multi-instance DVC support Mar 3, 2026

uchouT (uchouT) force-pushed the refactor/dvc branch 2 times, most recently from 7bc14b1 to 71d1115 Compare March 3, 2026 12:21

uchouT (uchouT) marked this pull request as ready for review March 4, 2026 20:23

Benoît Cortier (CBenoit) requested a review from Copilot March 5, 2026 05:17

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) March 5, 2026 05:17 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

uchouT (uchouT) commented Mar 5, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Show resolved Hide resolved

crates/ironrdp-dvc/src/server.rs Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Show resolved Hide resolved

uchouT (uchouT) and others added 3 commits March 10, 2026 02:01

feat(dvc): abstract listener trait in client-side

7c5a99b

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) force-pushed the refactor/dvc branch from a6fffe4 to 5f199ea Compare March 9, 2026 18:02

uchouT (uchouT) force-pushed the refactor/dvc branch from 5f199ea to ea96578 Compare March 12, 2026 13:52

Benoît Cortier (CBenoit) reviewed Mar 12, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

Benoît Cortier (CBenoit) requested a review from Copilot March 12, 2026 17:10

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) March 12, 2026 17:11 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

chore(dvc): docs update

d9f90f4

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) commented Mar 12, 2026

View reviewed changes

uchouT (uchouT) requested a review from Benoît Cortier (CBenoit) March 12, 2026 17:56

Benoît Cortier (CBenoit) reviewed Mar 13, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

chore(dvc): remove unnecessary Send trait bound

b1d24d7

Signed-off-by: uchouT <i@uchout.moe>

uchouT (uchouT) requested a review from Benoît Cortier (CBenoit) March 13, 2026 07:04

Benoît Cortier (CBenoit) reviewed Mar 13, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Outdated Show resolved Hide resolved

chore: rename

bdd54f9

Signed-off-by: uchouT <i@uchout.moe>

Benoît Cortier (CBenoit) reviewed Mar 13, 2026

View reviewed changes

uchouT (uchouT) requested a review from Benoît Cortier (CBenoit) March 13, 2026 07:44

Benoît Cortier (CBenoit) requested a review from Copilot March 13, 2026 08:00

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) March 13, 2026 08:00 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

docs(dvc): clarify silent overwrite behavior

cdd7dd4

Signed-off-by: uchouT <i@uchout.moe>

Benoît Cortier (CBenoit) approved these changes Mar 13, 2026

View reviewed changes

Benoît Cortier (CBenoit) enabled auto-merge (squash) March 13, 2026 13:37

Benoît Cortier (CBenoit) reviewed Mar 13, 2026

View reviewed changes

crates/ironrdp-dvc/src/client.rs Show resolved Hide resolved

Apply suggestion from @CBenoit

aefd10f

Benoît Cortier (CBenoit) merged commit 28e8628 into Devolutions:master Mar 13, 2026
10 checks passed

uchouT (uchouT) deleted the refactor/dvc branch March 13, 2026 13:56

devolutionsbot mentioned this pull request Mar 13, 2026

chore(release): prepare for publishing #1075

Open

Conversation

uchouT (uchouT) commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

older discussion

Uh oh!

Marc-Andre Lureau (elmarco) commented Mar 3, 2026

Uh oh!

uchouT (uchouT) commented Mar 3, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 3, 2026

Uh oh!

uchouT (uchouT) commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uchouT (uchouT) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uchouT (uchouT) commented Mar 9, 2026

Uh oh!

uchouT (uchouT) commented Mar 12, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uchouT (uchouT) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Mar 13, 2026

Uh oh!

Uh oh!

uchouT (uchouT) commented Mar 13, 2026

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

uchouT (uchouT) commented Mar 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Mar 13, 2026

Uh oh!

uchouT (uchouT) commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

uchouT (uchouT) commented Mar 3, 2026 •

edited

Loading

uchouT (uchouT) left a comment •

edited

Loading

Benoît Cortier (CBenoit) left a comment •

edited

Loading

uchouT (uchouT) commented Mar 13, 2026 •

edited

Loading