switch to tokio::process

[conradludgate]

Resolves #1652.

As mentioned in the issue, reqwest already depends on tokio, so this removes a large section of the new dependency tree. tokio::process also uses non-blocking IO for windows, which async-process does not

[conradludgate]

@microsoft-github-policy-service agree company="Neon, Inc."

[heaths]

This seems to take a hard dependency on tokio though, right? We intentionally do not have (unless there's a bug we missed) nor want a hard dependency on tokio (just default).

[svix-jplatte]

For non-wasm (which is the only thing this touches), you are already depending on tokio through reqwest and AFAIK there are really no good alternatives in the Rust ecosystem right now:

• isahc is nice but depends on libcurl which makes cross compilation harder, and it's not very actively maintained.
• Using hyper directly is an option, smol_hyper integrates it with smol as an alternative to tokio. But you'd have to write a bunch of (glue) code yourself on top if you need cookie handling, redirect handling, or such.

[arpad-m]

Yeah there is already an (optional) dependency on tokio through reqwest but most people will probably enable reqwest anyways, so they already pull in tokio. At least that's what we are doing as users of the azure SDK.

The current state pulls in the async-io crate unconditionally, which is IMO worse because that way, a lot of users end up depending on both async-io as well as tokio.

If you really want, it is possible to make the dependency optional by adding a crate-private module that either contains a reexport of tokio's Command struct, or if the tokio dependency is not enabled, contains a struct called Command with all the APIs we use, being a wrapper of std's Command (we cannot reexport std's Command directly because some APIs are async).

[heaths]

We actually do have use cases and budding implementation that will not use tokio but monoio in our feature branch. tokio will still be our default async runtime via reqwest.

That said, this branch will eventually be replaced but community-supported for some time. While no one can say whether the whole community is using tokio, it may be a safe assumption until proven otherwise and then we could implement @arpad-m's idea.

@ctaggart @demoray @jamesbell what do you think? For the stated reasons I'm all for replacing unmaintained async-process as mentioned in #1652. At this time for this branch, I wonder how strict we want to maintain a no-hard-dependency stance on tokio. Do you know of anyone using a different HTTP stack and/or async runtime?

[arpad-m]

@heaths I think this sounds like a great proposal and a good path forward. I think it makes sense to not have tokio be a hard dependency, but I feel for monoio it would make sense to not use async-io either.

What would be the next steps for this pull request to get merged?

[heaths]

@arpad-m could you add a trait much like we did for the HttpClient stack and do roughly what you suggested above so that the code uses tokio's Command by default but someone could replace it with another impl? It may not get used, but at least we're not painted into a corner and the official source we are working on will likely be pulling a lot of this source anyway, so it'll set a good precedent. In fact, the feature/track2 branch was branched from main but is getting refactored quite a bit. Still, much of the existing core implementation will be retained.

[arpad-m]

@heaths sounds good to me (although I'm not the PR author, my colleague Conrad is). For clarification, would you prefer the use of #[async_trait] instead of the language builtin async trait support, as the current MSRV is 1.74.0?

[heaths]

@heaths sounds good to me (although I'm not the PR author, my colleague Conrad is). For clarification, would you prefer the use of #[async_trait] instead of the language builtin async trait support, as the current MSRV is 1.74.0?

For now, let's stay consistent with elsewhere in the code and stick with #[async_trait]. I'd rather resolve that as a separate PR at some point in the future. The MSRV for the official branch is still somewhat up in the air.

[heaths]

main branch has been moved to legacy. Closing old PRs with no timely author response. Please reopen once you address feedback.