feat(dvc): add DVC named pipe proxy support#791
Conversation
Vladyslav Nikonov (pacmancoder)
changed the title
Coverage Report 🤖 ⚙️Past: New: Diff: -0.24% [this comment will be updated automatically] |
Vladyslav Nikonov (pacmancoder)
changed the title
| } | ||
|
|
||
| pub struct DvcPipeProxyFactory { | ||
| rdp_input_sender: mpsc::UnboundedSender<RdpInputEvent>, |
There was a problem hiding this comment.
note: It’s typically not ideal to use unbounded channels because there is no backpressure. You may consider a bounded channel and using async send or blocking_send instead. The sender will have to wait if the queue is full, creating (typically) necessary backpressure when the target system is overloaded.
There was a problem hiding this comment.
I am re-using the already existing RdpInputEvent channel, which is tokio::mpsc::Unbounded*, I agree that we need to change that in follow-up
| // Semaphore wrapper API itself by restricting handle usage: | ||
| // - release() method which calls ReleaseSemaphore inside (which is thread-safe). | ||
| // - borrow() method which returns a BorrowedHandle for waiting on the semaphore. | ||
| // - Handle lifetime is ensured by Arc, so it is always valid when used. |
There was a problem hiding this comment.
nitpick: I didn’t understand the point immediately. I think that this is not an argument explaining why the type is Send, but why it’s okay to implement Clone on it: if we were not using Arc, the handle would be closed on the first Semaphore that is dropped, invaliding the other semaphores and even causing the handle to be closed multiple times. But this is unrelated to thread safety, because that would be a problem even without Send. It’s still important to document that somewhere.
| // CreateSemaphoreW returns a valid handle on success. | ||
| Ok(Self { | ||
| // See `unsafe impl Send` comment. | ||
| #[allow(clippy::arc_with_non_send_sync)] |
There was a problem hiding this comment.
note: To myself, this is a bit suspicious. I need to verify.
|
Benoît Cortier (@CBenoit) are there any outstanding comments blocking this merge? 👀 |
There was a problem hiding this comment.
I think you are implementing a safe wrapper around Windows named pipes, but I know about two libraries that are doing that:
- tokio (async): https://docs.rs/tokio/latest/tokio/net/windows/named_pipe/index.html
- interprocess (blocking): https://docs.rs/interprocess/latest/x86_64-pc-windows-msvc/interprocess/os/windows/named_pipe/index.html
Did you consider them? Is there a reason for not using one of these instead? That would save us from a lot of unsafe code.
I just added a few extra comments, and I believe you didn’t address all of the existing comments either! |
I did not want to use |
|
Benoît Cortier (@CBenoit) approve please 😅 |
Vladyslav Nikonov (pacmancoder)
requested a review
from Benoît Cortier (CBenoit)
Sure thing – reviewing now. |
There was a problem hiding this comment.
praise: Good README.md, thank you!
| // Standard generic syntax is used instead if `impl` because of the following lint: | ||
| // > warning: lifetime parameter `'a` only used once | ||
| // | ||
| // Fixing this lint (use of '_ lifetime) produces compiler error. |
There was a problem hiding this comment.
issue? Maybe this comment is outdated
Vladyslav Nikonov (pacmancoder)
force-pushed
the
feat/now-proto-dvc-server
branch
from
3d9bf0c to
3d932a6
Compare
Indeed, I wouldn’t necessarily want |
### Changes - Make dvc named pipe proxy cross-platform (Unix implementation via `tokio::net::unix::UnixStream`) - Removed unsafe code for Windows implementation, switched to `tokio::net::windows::named_pipe` ### Testing This feature can be used in the [same way](#791) as on Windows, however instead of GUI test app there is new basic [CLI](Devolutions/now-proto#31) app
2 participants
This PR adds DVC proxy implementation based on named pipes (currently windows-only), a separate
fifioUnix implementation is needed.How to test:
cargo run -p ironrdp-client -- -u <USER> -p <PASS> <IP> --dvc-proxy Devolutions::Now::Agent=now-proto-GLOBALFFI is not yet implemented in this PR, it will be posted as separate follow-up PR.