Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch to tokio::process #1654

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

conradludgate
Copy link

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
Copy link
Author

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

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

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
Copy link

arpad-m commented May 14, 2024

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
Copy link
Member

heaths commented May 14, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async-io transitive dependency
4 participants