feat(dvc): add DvcChannelListener for multi-instance DVC support#1142
feat(dvc): add DvcChannelListener for multi-instance DVC support#1142uchouT (uchouT) wants to merge 6 commits intoDevolutions:masterfrom
Conversation
|
uchouT (@uchouT) lgtm, perhaps squash or reorganize some of your changes (PERF, ..) |
1daad55 to
7bc14b1
Compare
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>
7bc14b1 to
71d1115
Compare
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 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, |
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. |
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>
There was a problem hiding this comment.
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
DvcChannelListenertrait andOnceListeneradapter, refactorsDynamicChannelSet(moved fromlib.rstoclient.rs) to key active channels byDynamicChannelIdrather than by name - Adds
DrdynvcServer::create_channel()for mid-session server-side DVC channel creation - Removes the now-obsolete
DynamicChannelSetstruct fromlib.rs, replacingDynamicVirtualChannel::newwithfrom_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.
Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: uchouT <i@uchout.moe>
There was a problem hiding this comment.
Addressed Copilot's review. Benoît Cortier (@CBenoit)
Signed-off-by: uchouT <i@uchout.moe>
Resolves #1135
OnceListenerwrapper for pre-registered DVCs.create_channelforDrdynvcServer.older discussion
I try to implement the client-side without breaking the current API signatures by introducing
OnceListenerwrapper for pre-registered DVCs. Butget_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?