feat(windows): listen for DNS change events #4198
Conversation
ReactorScram
added
kind/feature
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
| // SAFETY: It's complicated. | ||
| // The callback here can cause a lot of problems. We pin the `Notify` object. | ||
| // We don't use an `Arc` because `Notify` already has only `&self` methods, and | ||
| // the callback has no way to free an `Arc`. |
There was a problem hiding this comment.
The callback can decrement the Arc using Arc::from_raw
There was a problem hiding this comment.
But when drop is called, we'll cancel the callback, which will forget the callback's cloned Arc, if the callback has not fired. We could check the Receiver to see whether we need to force the Arc to drop, but I thought it's simpler to imagine that the callback is taking a const borrow from Listener, and Drop enforces the lifetimes, and not that they're actually sharing ownership.
There was a problem hiding this comment.
but if we use the arc we don't need to unregister the callback, right?
There was a problem hiding this comment.
I don't understand. This one is the long-lived callback registered by RegisterWaitForSingleObject and unregistered in Drop by UnregisterWaitEx. If I didn't unregister the callback, it would leak the Arc and 1 or 2 Win32 handles.
| } | ||
|
|
||
| // Be careful with this, if you register twice before the callback fires, | ||
| // it will probably leak something. Check the MS docs for `RegNotifyChangeKeyValue`. |
There was a problem hiding this comment.
This should be an unsafe function
There was a problem hiding this comment.
Because of the constraint mentioned in that comment?
🤔 Since it's all &mut self, what if I enforce it at runtime with a flag callback_armed?
There was a problem hiding this comment.
I mean, it's okay to mark this as unsafe, specially since we use it only internally, the methods using it can make sure that they are enforcing the constaints.
There was a problem hiding this comment.
I don't think it's memory-unsafe, I was thinking of this sentence from the docs https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regnotifychangekeyvalue#remarks
Each time a process calls RegNotifyChangeKeyValue with the same set of parameters, it establishes another wait operation, creating a resource leak. Therefore, check that you are not calling RegNotifyChangeKeyValue with the same parameters until the previous wait operation has completed.
I might write a unit test for some of the registry stuff, there's no equivalent Unregister function, there's just closing the event handle. Even though it's only closed at the end of the process, it would be nice to know that editing the key won't break anything.
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Outdated
Show resolved
Hide resolved
…ezone/firezone into feat/windows/network-and-dns-changes
I don't want to delete the whole `dev.firezone.client` key in case some other test needs it
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Made this GA again since #4262 is GA and is blocked on Windows |
We don't usually set milestones or projects on PRs because it just makes the board too busy. Curious if you're finding it useful though? PRs will get auto-labeled by CI based on PR title so the changelog is nice, so it's not necessary to label them either, but labeling them doesn't hurt I suppose. |
|
@jamilbk Yeah I could use something to prioritize, since I accidentally ended up with IPC stuff that's post-GA, like this: #4277 If there's a different way I can do it without a milestone and without closing the PR (I'm worried I'd lose track of the branch and not be able to find the PR again when I'm ready to resume it), something else could work. Edit: Maybe editing the message at the top is enough, usually I put "paused" or "waiting on" or something already. So normally I try to finish any work-in-progress, starting with things closest to merging, and I want to remember that this PR needs my attention and the IPC PR doesn't. In general, having PRs and issues separate is a little tough - If I put a tasklist on an issue, I may not remember to check it before merging a PR. But I can't open a PR until the issue is investigated to a certain point, so there's a period where there isn't a good place to "land" more information about an issue. But that's how all Git forges work, I can't really think of a solution unless Github did a huge change to allow shared tasklists between issues and their PRs. I just like having those little micro-tasks that aren't big enough for a full issue, so that I know I won't forget anything. |
|
@ReactorScram No worries, I won't change PR milestones then |
|
|
||
| pub(crate) fn run_debug() -> Result<()> { | ||
| tracing_subscriber::fmt::init(); | ||
| let rt = tokio::runtime::Runtime::new()?; |
There was a problem hiding this comment.
Out of curiosity, why make a new runtime here and not just make it an async function?
There was a problem hiding this comment.
For the debug commands I don't want to start a runtime until I know which command it'll be, in case one of them needs something odd.
It's pretty rare that it matters, there's only a couple things (COM and Tauri) that could really care what thread they're on or what's happening.
There was a problem hiding this comment.
I see! I should have checked the code in more detailed, I was assuming that we use async fn main!
| tokio::select! { | ||
| _r = tokio::signal::ctrl_c() => break, | ||
| r = listener.notified() => r?, | ||
| } |
There was a problem hiding this comment.
Personally, I am not touching tokio::select with a 10ft pole because it has so many "interesting" features. If you only have two futures, it would suggest using futures::future::select instead which is trivial to understand when you look at the implementation.
If it is possible to remove the runtime above, you can even remove the ctrl_c listener here and just make this fn a loop of listening to be notified and compose it with ctrl_c at the outer layer of your app. I find listening to ctrl_c in main less surprising than several layers deep here.
YMMV :)
Tasks