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

Add feature to use async-std synchronization primitives instead of the ones from tokio #88

Closed
Fishrock123 opened this issue Feb 19, 2021 · 10 comments
Labels
A-core Area: Core / deadpool enhancement New feature or request RT-async-std Runtime: async-std S-blocked Status: Marked as blocked ❌ on something else.

Comments

@Fishrock123
Copy link

Fishrock123 commented Feb 19, 2021

http-client now depends on deadpool for keep-alive (connection pooling) support when using it's lightweight async-h1 backing implementation.

That now pulls in tokio, which is a bit undesirable when using a different async runtime. It would nice to have feature flags for that, to allow better interoperability with options with the async Rust ecosystem.

@bikeshedder
Copy link
Owner

The tokio dependency is only there for some sync primitives (tokio::sync::Mutex and tokio::sync::Semaphore) and tokio::time. The runtime is not used or depended on at all. Therefore you don't get the tokio runtime when using deadpool.

If you check the Cargo.toml of deadpool you will see that it only depends on the sync and time feature of tokio. Therefore only a very small subset of tokio is being used. The locking primitives of tokio::sync are executor independent and so is deadpool. I could replace those by something like async-lock. I prefer the tokio primitives as they are thoroughly tested and thanks to loom proven to be correct.

Tokio used to be split into various crates (e.g. tokio-sync). Just think of tokio with the default features disabled and sync enabled as a 1:1 replacement of this former tokio-sync crate. The tokio sync primitives are in fact runtime independent.

The only issue I know about is the use of tokio::time. Timeouts currently only work with tokio (See #68). I was hesitant to add timeouts in the first place and soon after adding them realized that they render this feature to be runtime specific (See #9). This is something I want to take care of very soon. I was hoping for async_executors to add timeout support (See najamelan/async_executors#3) but that hasn't happened, yet.

@bikeshedder bikeshedder added A-core Area: Core / deadpool documentation Improvements or additions to documentation enhancement New feature or request labels Feb 20, 2021
@bikeshedder
Copy link
Owner

I was wondering if you were just worried about the compatibility with the async-std runtime or wanted to see deadpool support multiple Mutex/RwLock/Semaphore implementations to cut down the build size?

I added the documentation tag to this issue as I should add this to the FAQ: The use of tokio::sync does not mean deadpool depends on the tokio runtime features. The README does state that deadpool is compatible with any executor. The tokio dependency could deceive people into believing that is also depends on the tokio runtime which is not true unless using timeouts while #68 is not fixed.

@Fishrock123
Copy link
Author

I am not super worried about compatibility, I don't think there is or has been an issue with that.

I would like tokio to be excluded from the build and compilation if possible. If we don't need it, we don't need it.
It would likely be surprising to anyone that the 'most async-std' client for Surf more or less directly pulls in tokio as a build dep.

@bikeshedder
Copy link
Owner

If we don't need it, we don't need it.

That's the thing. It is needed.

The core of deadpool needs semaphores as synchronization primitives.

For the semaphore implementation there are currently three popular options:

I checked async-lock and it currently provides no way to "forget" a permit. I could be using mem::forget for that but there is also no API to add new permits to an existing semaphore. This renders this semaphore implementation unusable for the purposes of deadpool.

futures-intrusive does have a Releaser with a disarm method and also provides a Semaphore::release method to add new permits. It can't be used as a drop-in replacement though as it doesn't implement the concept of being closed. This could be emulated however.


I compared the binary size of these three semaphore implementations with a simple program that creates a semaphore, aquires it and releases it in the end:

tokio

file size: 3,201,952 bytes

cargo tree:

rust-semaphore v0.1.0 (/home/bikeshedder/projects/foss/rust-semaphore)
└── tokio v1.4.0
    └── pin-project-lite v0.2.6
    [build-dependencies]
    └── autocfg v1.0.1

futures-intrusive

file size: 3,206,112 bytes

cargo tree:

rust-semaphore v0.1.0 (/home/bikeshedder/projects/foss/rust-semaphore)
└── futures-intrusive v0.4.0
    ├── futures-core v0.3.13
    ├── lock_api v0.4.3
    │   └── scopeguard v1.1.0
    └── parking_lot v0.11.1
        ├── instant v0.1.9
        │   └── cfg-if v1.0.0
        ├── lock_api v0.4.3 (*)
        └── parking_lot_core v0.8.3
            ├── cfg-if v1.0.0
            ├── instant v0.1.9 (*)
            ├── libc v0.2.92
            └── smallvec v1.6.1

async-lock

file size: 3,196,032

rust-semaphore v0.1.0 (/home/bikeshedder/projects/foss/rust-semaphore)
└── async-lock v2.3.0
    └── event-listener v2.5.1

TL;DR - Deadpool uses tokio::sync and has the default-features disabled. The base build size (without any dependencies) for a binary is 3,189,088 bytes on my system. So the tokio::sync::Semaphore adds 12,864 bytes, futures-intrusive adds 17,024 bytes and async-lock adds 6,944 bytes.

In its current form async-lock is ununsable for deadpool at it doesn't provide acces to the low level features. I would need to implement a custom semaphore based on the event-listener crate which is internally used by async-lock.

I need to emphasize this once more: By using deadpool you are not using the entire tokio crate. You are only using a very small subset of tokio. For the tokio dependency the default features are disabled and the sync feature is cherry-picked.

@Fishrock123
Copy link
Author

This sounds like missing functionality in async-lock.

async-lock would actually add zero new dependencies or size on a stack already using async-std, given that crate is already used by async-std itself: https://github.com/async-rs/async-std/blob/35f768166436112db97224e823b4ee610c81d6d6/Cargo.toml#L55

@bikeshedder
Copy link
Owner

You can check with cargo tree --format "{p} {f}" that only a small subset of tokio is actually being used.

While checking async-lock I found another issue why I don't want to use it. It is not fair.

futures-intrusive does support fair semaphores but it increases the build size even more if you're not using it already.

Tokio semaphores are fair and have all the features needed by deadpool. Also tokio is the most popular async runtime at the moment. So it won't increase the build size for most deadpool users. And for those not using tokio already it's a single digit kb of extra build size.

I looked into writing a custom semaphore implementation based on event-listener for the fun of it, but don't feel very confident in rolling my own synchronization primitive as it is basically the most critical thing in deadpool.

@Fishrock123
Copy link
Author

Also tokio is the most popular async runtime at the moment. So it won't increase the build size for most deadpool users.

Sure, myself and the others here who reacted to the comment are not necessarily the common case user. It would still be nice to have less dependencies if there is already something from an executor ecosystem which should theoretically cover this use case. As I said, it sounds like there is deficiency issues with async-lock.

@bikeshedder
Copy link
Owner

The fix for #68 just landed in bcbe567

I might be adding a swappable semaphore implementation in the future using the same technique once there is a fair semaphore implementation available that also supports the low level methods for adding new permits and some kind of closing it for implementing a clean shutdown.

I'm currently not planning to work on this as the tokio semaphore works fine for both users of tokio and any other runtime.

I'm keeping an eye on this but unless async-std and/or async-lock implements a more powerful/complete semaphore implementation I'm marking this issue as blocked.

@bikeshedder bikeshedder added S-blocked Status: Marked as blocked ❌ on something else. and removed documentation Improvements or additions to documentation labels Apr 6, 2021
@Fishrock123
Copy link
Author

That's fair. I don't think anyone is in a rush here, just that this would be nice to have.

@Fishrock123 Fishrock123 changed the title Add feature to use async-std rather than tokio Add feature to depend on async-std rather than tokio Apr 7, 2021
@bikeshedder bikeshedder changed the title Add feature to depend on async-std rather than tokio Add feature to use async-std synchronization primitives instead of the ones from tokio Jun 1, 2021
@bikeshedder bikeshedder added the RT-async-std Runtime: async-std label Sep 29, 2021
bors added a commit to rust-lang/crates.io that referenced this issue Oct 1, 2021
…rbo87

Show features of dependencies

I keep getting asked why my crate depends on `tokio` besides it just needing a small subset of features: bikeshedder/deadpool#88

This PR adds the list of features used from each dependency to the dependencies tab:

![image](https://user-images.githubusercontent.com/1112569/114195470-c5ce5800-9950-11eb-88c0-8e607a29213d.png)
@bikeshedder
Copy link
Owner

As I don't see another fair and sound async semaphore implementation being available and I don't plan on writing my own I'm closing this as blocked:

@bikeshedder bikeshedder closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core / deadpool enhancement New feature or request RT-async-std Runtime: async-std S-blocked Status: Marked as blocked ❌ on something else.
Projects
None yet
Development

No branches or pull requests

2 participants