Fix split tunneling failing due to slow syscalls#10054
Conversation
MarkusPettersson98
left a comment
There was a problem hiding this comment.
@MarkusPettersson98 reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on dlon).
talpid-core/src/split_tunnel/windows/driver.rs line 399 at r1 (raw file):
/// Call `f` and log a warning if it takes longer than `threshold`. fn log_if_slow<T>(label: &str, threshold: Duration, f: impl FnOnce() -> T) -> T {
Suggestion:
fn log_at_timeout<T>(label: &str, threshold: Duration, f: impl FnOnce() -> T) -> Ttalpid-core/src/split_tunnel/windows/volume_monitor.rs line 119 at r1 (raw file):
} let mut known_state_guard = known_state.lock().unwrap(); let paths_guard = paths.lock().unwrap();
If the ordering is important here, we should make sure to document it.
Code quote:
let mut known_state_guard = known_state.lock().unwrap();
let paths_guard = paths.lock().unwrap();talpid-core/src/split_tunnel/windows/windows.rs line 62 at r1 (raw file):
let buffer_size = unsafe { GetFinalPathNameByHandleW(raw_handle, ptr::null_mut(), 0u32, VOLUME_NAME_NT) } as usize;
Is this safety comment really true? File implements AsRawHandle, but so does JoinHandle.
If we only expect get_final_path_name_by_handle to work on files, should we accept a file as an argument?
Code quote:
// SAFETY: file is a valid file handle
let buffer_size =
unsafe { GetFinalPathNameByHandleW(raw_handle, ptr::null_mut(), 0u32, VOLUME_NAME_NT) }
as usize;
dlon
left a comment
There was a problem hiding this comment.
@dlon made 2 comments and resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on MarkusPettersson98).
talpid-core/src/split_tunnel/windows/volume_monitor.rs line 119 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
If the ordering is important here, we should make sure to document it.
Done! The reason is that we acquire them in a different order elsewhere, so there's a potential deadlock.
talpid-core/src/split_tunnel/windows/windows.rs line 62 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Is this safety comment really true? File implements AsRawHandle, but so does JoinHandle.
If we only expect get_final_path_name_by_handle to work on files, should we accept a file as an argument?
Good catch! Switched to &File.
MarkusPettersson98
left a comment
There was a problem hiding this comment.
@MarkusPettersson98 reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
talpid-core/src/split_tunnel/windows/windows.rs line 62 at r1 (raw file):
Previously, dlon (David Lönnhager) wrote…
Good catch! Switched to
&File.
✨
Pre 2026.1, the daemon would just exit if
set_paths_sync(setting the initial paths to use for split tunneling) failed during startup. This caused the daemon to be restarted and hopefully succeed during later attempts. In 2026.1, the error would instead just cause the daemon to disable split tunneling, leaving it permanently disabled.This seems to happen because
CreateFile(OpenOptions::new().read(true).open(...)) can take a very long time to complete, soset_paths_synchits a 5-second timeout.This PR adds a long, separate timeout for resolving the paths. This could probably be refactored in two ways: (1) Add an
initializingstate, and (2) make theerrorstate recoverable in certain cases.Fix DES-2914
This change is