fix(dashboard): 🐛 Fix SteamVR launch fail for dangling ADB process #2757
Conversation
zmerp
changed the title
zmerp
force-pushed
the
kill-adb-on-launch
branch
3 times, most recently
from
abe54d8 to
79ddef4
Compare
zmerp
force-pushed
the
kill-adb-on-launch
branch
from
79ddef4 to
3ea68c4
Compare
|
Tested by @0JXI0, ready to review |
| // The ADB server might be left running because of a unclean termination of SteamVR | ||
| let local_adb_path = crate::get_filesystem_layout().local_adb_exe(); | ||
| for proc in | ||
| sysinfo::System::new_all().processes_by_name(OsStr::new(&afs::exec_fname("adb"))) | ||
| { | ||
| if let Some(proc_path) = proc.exe() { | ||
| if proc_path == local_adb_path { | ||
| proc.kill(); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I mean adb intentionally has the server architecture, I find killing it to be a bit of a meh thing, but I guess we're already doing it in case of a normal shutdown, so what gives.
But what happens when alvr picks up/launches a system wide installed adb server? This wouldn't handle that and in that scenario killing adb is a bad option and the logic to unregister the old port forwardings would work for both local and system wide adb (tho that would probably require quite a bit of more adb specific logic).
There was a problem hiding this comment.
Also adb kill-server does the same thing but more cleanly, so maybe that's a better option?
There was a problem hiding this comment.
@E1int What do you think? Would you be willing to work on this?
| fn get_adb_path(layout: &Layout) -> Option<String> { | ||
| fn get_adb_path(layout: &afs::Layout) -> Option<String> { | ||
| get_os_adb_path().or(get_local_adb_path(layout)) | ||
| } | ||
|
|
||
| fn get_os_adb_path() -> Option<String> { | ||
| let name = get_executable_name().to_owned(); | ||
| let name = afs::exec_fname("adb").to_owned(); | ||
|
|
||
| get_command(&name, &[]).output().is_ok().then_some(name) | ||
| } | ||
|
|
||
| fn get_local_adb_path(layout: &Layout) -> Option<String> { | ||
| let path = get_platform_tools_path(layout).join(get_executable_name()); | ||
| fn get_local_adb_path(layout: &afs::Layout) -> Option<String> { | ||
| let path = layout.local_adb_exe(); | ||
|
|
||
| path.try_exists() | ||
| .is_ok_and(|e| e) | ||
| .unwrap_or(false) | ||
| .then(|| path.to_string_lossy().to_string()) | ||
| } |
There was a problem hiding this comment.
ik this isn't your code but these should probably be consolidated into one function
| pub fn local_adb_dir(&self) -> PathBuf { | ||
| self.executables_dir.join("platform-tools") | ||
| } |
There was a problem hiding this comment.
I don't think this function should exist, it's only used locally and what it's amounting to is a hard-coded path part anyway ("platform-tools/adb") and thus will likely also never be used in any other capacity, which should only really be one function.
|
As things stand, probably this PR shouldn't be merged. I could get some help making another PR fixing this the proper way, as suggested by @The-personified-devil |
|
I mean I can take a look at it tmrw, but I'm not entirely sure how to ideally handle this yet |
|
@The-personified-devil We would need to detect if the current alive adb server is spawned from the current process or not. if it's the local ADB installation it's always spawned by the driver, if not then it's safe to let the ADB instance live because it will not block SteamVR. the crate sysinfo should be able to tell if the parent process is SteamVR (check if adb is spawned by the current process) |
No description provided.