Clean up dependencies in talpid-openvpn

[MarkusPettersson98]

Some cleanups to dependencies in talpid-openvpn. The major one is replacing duct with tokio::process::Command for spawning an OpenVPN process. The minor cleanups make once_cell a proper workspace-dependency and replacing the is_terminal crate with std::io::IsTerminal.

---

This change is 

[linear]

DES-285 Rewrite talpid-openvpn to be more async

The current implementation of spawning and monitoring OpenVPN in talpid-openvpn uses blocking subprocess libraries (duct). But it immediately wraps it all up in async wrappers and is used in an async fashion. If we just migrated all of this to use tokio::process::Command we could drop the dependency on duct and have one async→sync layer removed.

[dlon]

Reviewed 13 of 17 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-openvpn/src/lib.rs line 814 at r2 (raw file):

impl ProcessHandle for OpenVpnProcHandle {
    fn wait(&self) -> io::Result<ExitStatus> {
        let handle = tokio::runtime::Handle::current();

Was it not possible to replace this with an async method? Having this many sync-async boundaries is making things messier.

---

talpid-openvpn/src/process/openvpn.rs line 415 at r2 (raw file):

    fn kill(&self) -> io::Result<()> {
        log::warn!("Killing OpenVPN process");
        let rt = tokio::runtime::Builder::new_current_thread()

Was it not possible to replace this with an async method? Having this many sync-async boundaries is making things messier.

I'm not sure we even need the StoppableProcess trait. We're only implementing it on OpenVpnProcHandle, so we might as well just add async methods to replace these.

[MarkusPettersson98]

Reviewable status: 16 of 19 files reviewed, all discussions resolved (waiting on @dlon)

---

talpid-openvpn/src/lib.rs line 814 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Was it not possible to replace this with an async method? Having this many sync-async boundaries is making things messier.

done

---

talpid-openvpn/src/process/openvpn.rs line 415 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Was it not possible to replace this with an async method? Having this many sync-async boundaries is making things messier.

I'm not sure we even need the StoppableProcess trait. We're only implementing it on OpenVpnProcHandle, so we might as well just add async methods to replace these.

done

I didn't touch StoppableProcess this time, but I did have to convert it to an async trait.

[dlon]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-openvpn/src/lib.rs line 489 at r3 (raw file):

            let handle = self.runtime.clone();
            handle.spawn(async move {

I'm sure we could make this whole wait function async. But I'll not increase the scope of this PR. It's already a big improvement. :)

---

talpid-openvpn/src/process/openvpn.rs line 441 at r3 (raw file):

    async fn has_stopped(&self) -> io::Result<bool> {
        let exit_status = self.inner.lock().await.try_wait();
        match exit_status {

Nit: Ok(exit_status?.is_some())

---

talpid-openvpn/src/process/stoppable_process.rs line 51 at r3 (raw file):

            return Ok(true);
        }
        thread::sleep(POLL_INTERVAL_MS);

Use tokio::time::sleep. This blocks the runtime.

[dlon]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-openvpn/src/process/stoppable_process.rs line 51 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Use tokio::time::sleep. This blocks the runtime.

I'm tunnel visioning a bit here. Can't we use tokio::time::timeout() on Child::wait() instead of polling?

[MarkusPettersson98]

Reviewable status: 17 of 19 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

talpid-openvpn/src/lib.rs line 489 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm sure we could make this whole wait function async. But I'll not increase the scope of this PR. It's already a big improvement. :)

It would be nice, the wait function is overly complicated due to it not being async. Maybe due for next cleanup 🧹

---

talpid-openvpn/src/process/openvpn.rs line 441 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Ok(exit_status?.is_some())

done

---

talpid-openvpn/src/process/stoppable_process.rs line 51 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm tunnel visioning a bit here. Can't we use tokio::time::timeout() on Child::wait() instead of polling?

tokio::time::timeout did the trick 👌

[dlon]

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 18 of 19 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

talpid-openvpn/src/process/stoppable_process.rs line 51 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

tokio::time::timeout did the trick 👌

I think this won't work. has_stopped will return immediately. It won't actually wait for the child process to stop. We need to use timeout on wait, if possible.

[MarkusPettersson98]

Reviewable status: 16 of 19 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

talpid-openvpn/src/process/stoppable_process.rs line 51 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think this won't work. has_stopped will return immediately. It won't actually wait for the child process to stop. We need to use timeout on wait, if possible.

Done.

[dlon]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

talpid-openvpn/src/lib.rs line 812 at r5 (raw file):

impl ProcessHandle for OpenVpnProcHandle {
    async fn wait(&self) -> io::Result<ExitStatus> {
        crate::process::stoppable_process::StoppableProcess::wait(self).await

How about just adding a wait() method on OpenVpnProcHandle directly and calling that?

[MarkusPettersson98]

Reviewable status: 16 of 20 files reviewed, all discussions resolved (waiting on @dlon)

---

talpid-openvpn/src/lib.rs line 812 at r5 (raw file):

Previously, dlon (David Lönnhager) wrote…

How about just adding a wait() method on OpenVpnProcHandle directly and calling that?

done

[dlon]

Reviewed 7 of 7 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Ready to be merged, since e201d1a and f88689d was merged to main