-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Control if redis uses async-std or tokio via feature flags #4
Control if redis uses async-std or tokio via feature flags #4
Conversation
I have noticed a regression when dropping the lock guard, where when dropping it, it locks up tokio when doing the futures::executor::block_on call to unlock. I will attempt to conditionally compile either Edit: But now I realize that this creates a situation where we must either ensure mutually-exclusive feature-flags, so as to not have both sections of code run, causing the same regression if a transitive dependency sets At the moment, because there isn't a good story for mutually-exclusive features in cargo, I'm going to opt to compile out |
I added an I would prefer Now, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for taking the time to research and optimize this library. your brief on tokio blocking makes sense.
to prevent users from getting compilation errors when using no-default-features
in their Cargo.toml, you should add this guard before LockManager
: #[cfg(any(feature = "async-std-comp", feature = "tokio-comp"))]
additionally, please run a cargo fmt
to satisfy ci
thanks :)
Hello, thanks for working with me to integrate this change. I've done my best to make the changes you requested. I differed by putting |
I see the issue, the test suite runs with |
This is imperfect, because the test suite doesn't cover the currently failing test now. I figure I should extend the workflow to test with different feature flags, but I'm unsure exactly how you want that to be done. I figure having a single stable-toolchain job without the |
good point, i am happy to accept this proposed |
I saw that this project was pulling in async-std in as a dependency. My project didn't otherwise use async-std, instead using tokio, and would prefer to avoid async-std if possible.
So to preserve the current behavior, I pulled out
async-std-comp
into its own feature flag and made it the default. I then tested this against my project that was already using rslock, settingdefault-features = false
andfeatures = ["tokio-comp"]
and found that it was functionally the same, and saved me from compiling ~15 dependencies I evidently didn't need.Since you directly depend on tokio anyways, I'm curious if the default shouldn't be
tokio-comp
instead ofasync-std-comp
to avoid pulling in both, since downstream projects likely only include one or the other. Thoughts?