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

Make tokio an optional dependency for core module #95

Closed
wants to merge 1 commit into from
Closed

Make tokio an optional dependency for core module #95

wants to merge 1 commit into from

Conversation

sunng87
Copy link

@sunng87 sunng87 commented Apr 9, 2021

At the moment, tokio is a required dependency even if we are using async-std. This patch make it optional to clean up dependency tree.

@bikeshedder
Copy link
Owner

tokio/sync is a required dependency. (not tokio, as explained in #88)

tokio used to be split into tokio-sync, tokio-util, tokio-io, etc. Currently it's an all-in-one crate with feature flags. If you add tokio = { version = "1", features = ["sync"] } you are only getting tokio::sync and not the rest of it. The tokio runtime is NOT included.

deadpool needs tokio::sync::Semaphore to function. That dependency is not optional.

The CI tests do pass because they are run with --all-features and therefore tokio is enabled as well.

@bikeshedder
Copy link
Owner

If you want to make tokio/sync optional you first need to create a new Semaphore implementation that supports the same features as tokio::sync::Semaphore does:

  • fair
  • low level (adding and removing permits without a guard)
  • closing (used for shutdown purposes)

The semaphore implementation from async-lock (which is reexported by async-std) supports neither of those features. It's rather primitive to be honest.

The semaphore from futures-intrusive could be used, but that comes with even more dependencies.

tokio::sync::Semaphore is currently the only choice available that has all the required features.

@bikeshedder bikeshedder closed this Apr 9, 2021
@bikeshedder
Copy link
Owner

I've added tests to the CI that should cover this. If you rebase your PR to the latest version of the master branch the checks should now be failing.

@sunng87
Copy link
Author

sunng87 commented Apr 9, 2021

Understand. Thank you for clarification.

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.

2 participants