feat(client): add RDM host handler and improve RDM client API #57
Conversation
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
vnikonov-devolutions
force-pushed
the
feat/rdm-messages
branch
from
d8f0362 to
04eaabd
Compare
There was a problem hiding this comment.
Pull request overview
This PR simplifies the RDM (Remote Desktop Manager) client API and adds a new host-side implementation for processing RDM messages. The changes remove client-side session state management in favor of a global event-driven approach, add proper disposal patterns with cancellation token support, and introduce a new NowRdmHost for handling RDM messages on the server side.
Key changes:
- Simplified RDM client API by removing
RdmSessionstate object and replacing per-session handlers with a global session notification handler - Implemented
NowRdmHostfor server-side RDM message processing with event-based API and pipe transport - Added cancellation token support and IDisposable implementation to transport interfaces and cleanup logic
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
WorkerCtx.cs |
Added try-finally block for cleanup and disposal, moved cleanup logic to new RunImpl method, replaced per-session tracking with global handler |
CommandSetRdmSessionNotifyHandler.cs |
New command to register global session notification handler |
CommandRdmSessionStart.cs |
Simplified to remove session object tracking, now just sends message |
RdmTypes.cs |
Converted classes to primary constructors, changed RdmStartParams to fluent API, simplified RdmSessionStartParams |
RdmSession.cs |
Removed entire session state management class |
NowRdmHostPipeTransport.cs |
New server-side pipe transport implementation with cancellation token support |
NowRdmHostException.cs |
New exception type for RDM host operations |
NowRdmHost.cs |
New host implementation with pipe server, capability negotiation, and event-driven message processing |
NowClientPipeTransport.cs |
Added cancellation token parameter to Read and IDisposable implementation |
NowClient.cs |
Added disposal on connection failure, new global session handler setter, refactored RdmSessionStart to not return session object, added RdmSessionFocus/Close methods |
NowChannelTransport.cs |
Added cancellation token support to read methods and IDisposable implementation |
INowTransport.cs |
Added cancellation token parameter to Read method and IDisposable interface |
Program.cs |
Updated CLI to use new fluent API for RdmStartParams |
Comments suppressed due to low confidence (1)
dotnet/Devolutions.NowClient/src/Worker/WorkerCtx.cs:291
- This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
public required NowChannelTransport NowChannel;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dotnet/Devolutions.NowClient/src/Worker/CommandSetRdmSessionNotifyHandler.cs
Outdated
Show resolved
Hide resolved
|
@vnikonov-devolutions I've opened a new pull request, #58, to work on those changes. Once the pull request is ready, I'll request review from you. |
Copilot AI
added a commit
that referenced
this pull request
Dec 25, 2025
…PI consistency Co-authored-by: vnikonov-devolutions <246051166+vnikonov-devolutions@users.noreply.github.com>
…safety, and API improvements (#58) Addresses all code review feedback on PR #57 regarding resource management, thread safety, exception handling, and API consistency. ## Changes Made ### Resource Management & Disposal - Replaced try-finally disposal pattern with C# `using` statements for three disposable resources in `NowRdmHost.Run()`: - `NamedPipeServerStream` - `NowRdmHostPipeTransport` - `NowChannelTransport` - Fixed disposal order issue by relying on using statements to handle the disposal chain automatically - Removed double disposal of transport objects ### Thread Safety - Added lock-based synchronization (`_transportLock`) for the `_transport` field - Protected all access to `_transport` in `Run()`, `SendAppNotify()`, `SendSessionNotify()`, and `IsConnected` property - Prevents race conditions when transport is accessed from multiple threads ### Exception Handling - Added try-catch blocks around all event handler invocations (`OnAppStart`, `OnAppAction`, `OnSessionStart`, `OnSessionAction`) - Prevents unhandled exceptions in user code from terminating the message processing loop - Errors are logged to `Console.Error` with context about which handler failed ### API Consistency - Replaced getter methods (`GetSessionId`, `GetConnectionId`, `GetConnectionData`) with public properties in `RdmSessionStartParams` - Added XML documentation for all public properties - Updated all usages in `NowClient.cs` to use properties instead of methods - Made API consistent with other parameter classes (`RdmVersion`, `RdmCapabilityInfo`) ### Documentation Fixes - Fixed method name in documentation: "RunServerLoop()" → "Run()" - Fixed typo: "if always" → "is always" ### Performance Improvements - Removed unnecessary async/await in `CommandSetRdmSessionNotifyHandler` and `CommandSetRdmAppNotifyHandler` - Both now return `Task.CompletedTask` directly ## Testing - ✅ Build successful with no warnings or errors - ✅ Code review completed - ✅ CodeQL security scan passed with no vulnerabilities The changes improve code quality by following C# best practices, eliminating potential race conditions, preventing crashes from user code exceptions, and providing a consistent and well-documented API. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: vnikonov-devolutions <246051166+vnikonov-devolutions@users.noreply.github.com>
vnikonov-devolutions
changed the title
vnikonov-devolutions
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
RdmSessionstate, this will be offloaded completely to RDM implementation.Those are C#-only changes, Rust & proto itself are unchanged.