Add async_std support #281

DavidBM · 2020-02-05T14:06:35Z

Based in the conversation here: #280

This branch comes from https://github.com/Marwes/redis-rs/tree/combine-4 If you merge this branch you will be merging this pull request too: #272

When I needed to duplicate the method and it was public, I tried to create two variants (one with _tokio suffix and the other with _async_std) and leave the original method calling the _tokio variant. Still, very provably this mean breaking changes.

All test pass in my pc (Pop_os 19.10)

Let me know if I need to change anything!

DavidBM · 2020-02-05T14:31:56Z

Fixing windows build

Marwes

Diff without most of the changes in #272 https://github.com/mitsuhiko/redis-rs/pull/281/files/a69d03bcf17592f48f3cc10fe4a694f1800c3946..b3163c1aeb92d88f7cbf2565a1a9d54a7ff9f042

Cargo.toml

src/client.rs

src/aio.rs

Marwes · 2020-02-05T14:59:45Z

tests/test_async_async_std.rs

@@ -0,0 +1,310 @@
+use redis;


Would be nice to not duplicate the tests here, not critical to fix however.

(Maybe an environment variable could switch which method call the support module uses?)

Yeah, we can flag them. I duplicated them as a fast way to test everything with async_std. I think we can add an argument for that

For now this also acts as an example for async-std.
When we refactor common parts out we should add a minimal example (just as the await-async one) for async-std usage

src/aio.rs

DavidBM · 2020-02-05T15:30:15Z

ok. I'm looking into the suggestions. I don't know if I will have time today, if not, I hope I can do it tomorrow.

mitsuhiko · 2020-02-05T18:54:46Z

I have to say that having methods end in _tokio etc. is a pretty awful experience. I wonder if we can hide this behind some traits at least. It's quite frustrating that we need to do this at all to be honest.

DavidBM · 2020-02-05T19:52:01Z

We discussed about that in #280 and didn't find a way to do it. I don't know how to make Rust to choose the correct implementation depending of the reactor. I agree that it would be much better to hide it.

Marwes · 2020-02-05T20:03:05Z

We could actually do something like this.

#[cfg(any(feature = "tokio", feature = "async-std")]
fn get_async_connection(&self) -> RedisResult<Connection> {
    #[cfg(all(feature = "tokio", not(feature = "async-std"))]
    {
        // Call tokio connect
    }

    #[cfg(all(not(feature = "tokio"), feature = "async-std")]
    {
        // Call async-std connect
    }

    #[cfg(all(feature = "tokio", feature = "async-std")]
    {
        if tokio::runtime::Handle::try_current().is_ok() {
            // Call tokio connect
        } else {
            // Call async-std connect
        }
    }
}

Complicates internally but simplifies for users.

DavidBM · 2020-02-05T21:00:34Z

Hi!

I think I made all the changes proposed.

I may say that with the features flag and the cfg attributes, the function MultiplexedConnection::create_connection is getting very long. Adding a third executor would make that function very very long.

About the proposed method, I think it may work. I will try.

DavidBM · 2020-02-05T21:13:48Z

I tried to add something like what you proposed @Marwes

I'm not making tokio an optional feature because the function MultiplexedConnection::create_connection is getting very very long and making Tokio optional would make it too much.

I named the feature for async_std "async-std-aio" because seems tat features and dependencies names cannot collide.

DavidBM · 2020-02-05T21:17:25Z

Going to sleep, let me know tomorrow your thoughts!

Marwes · 2020-02-06T17:28:17Z

I named the feature for async_std "async-std-aio" because seems tat features and dependencies names cannot collide.

As a workaround you can rename the package and then add your own async-std feature.

async-std-dep = { version = "*", package = "async-std" }

DavidBM · 2020-02-07T10:14:32Z

Done!

DavidBM · 2020-02-10T19:07:39Z

@mitsuhiko @Marwes Are you happy with this PR?

If yes, I'm planning some changes on my library, when do you think (more or less) that you will be able to merge this PR?

Marwes · 2020-02-13T14:22:56Z

@DavidBM Did you see DavidBM#1 ? LGTM, modulo formatting failing on CI which should be fixed.

It is based on #280 which I am also fine with, but ideally someone else should be fine with that PR as well :)

DavidBM · 2020-02-13T14:30:23Z

Nice! That makes a lot of sense!

I didn't see it. I will make tokio optional and then request the review again. Thanks!

DavidBM · 2020-02-13T17:16:20Z

At the end I found a better way (in my opinion) for naming the features.

Tokio and Async_std are optional. If aio is enabled without them, there is a compilation error. In any case, I don't think that anyone runs in that problem. The code should be ready for adding new reactors in the future.

Later I will check the errors in the builds and fix them.

DavidBM · 2020-02-13T22:41:29Z

Ok, all test pass. Pull request ready to go.

badboy · 2020-02-20T19:39:38Z

I merged #272, so if you rebase this it would make the review easier (unfortunately GitHub is awful in simply hiding or separating these commits easily).
If you're at it squashing those "fix" commits into their appropriate parent commits would help keep changes per commit reviewable.

(Don't worry if that is too much work, we can get through this somehow, and of course already thank you for investing the time!)

badboy

I agree with @mitsuhiko that exposing _tokio and _async_std methods is indeed suboptimal, but I don't see a way around it either.
We could of course make the features mutual-exclusive, but due to how cargo features work that might lead to an even worse experience for users. Hmmm, I don't have a solution here.

I think the code here is ok, see some inline nits though.

Cargo.toml

src/aio.rs

badboy · 2020-02-20T21:39:39Z

tests/test_async_async_std.rs

@@ -0,0 +1,310 @@
+use redis;


For now this also acts as an example for async-std.
When we refactor common parts out we should add a minimal example (just as the await-async one) for async-std usage

src/aio.rs

before rebase

fix windows build fix missing config attributes for aio

fix tests

fix windows build

Reduce branching in create_connection

- replaces the "aio" default feature by the "tokio-comp" feature. - async-std-comp and tokio-comp features automatically activate aio feature - enabling aio without tokio-comp or async-std-comp will result in a compilation error

fix feature flags names fix windows test format code enable async_std and tokio support by default

sync with master

Code review

DavidBM · 2020-02-22T17:49:40Z

Hi! I made the requested changes. I see that github is showing many commits, still, the diff more clear now. Let me know if anything else is needed! And thanks for taking the time to do a code review!

badboy

With the one question left I think this is ready to be merged.
I'll give it a final look tomorrow then and we can land this (and maybe even cut a release)

src/aio.rs

DavidBM · 2020-02-23T13:35:08Z

Done! :)

DavidBM · 2020-03-01T21:44:52Z

I fixed an error caused by a new code that was not adapted to the async_std and tokio.

Marwes · 2020-03-02T07:30:28Z

Looks like a formatting error still, looks good otherwise.

DavidBM · 2020-03-02T08:23:16Z

Ohh, sorry. I fixed and left it running without checking it. Should be fixed now.

badboy · 2020-03-02T08:38:25Z

Woops. Managed to not land it last week. I promise to get to it today.

badboy · 2020-03-03T09:33:42Z

Thank you so much!

DavidBM · 2020-03-03T13:26:30Z

Thanks to you all! I have learned a lot during the process!

Increase timeout for cluster creation.

update types & function names with Tokio suffix

1e44f84

DavidBM changed the title ~~Add support for .~~ Add async_std support Feb 5, 2020

DavidBM requested a review from Marwes February 5, 2020 14:07

DavidBM mentioned this pull request Feb 5, 2020

Making the library executor agnostic (making Tokio optional) #280

Closed

Marwes requested changes Feb 5, 2020

View reviewed changes

DavidBM requested a review from Marwes February 13, 2020 13:44

badboy self-requested a review February 16, 2020 22:16

badboy requested changes Feb 20, 2020

View reviewed changes

badboy reviewed Feb 20, 2020

View reviewed changes

src/aio.rs Outdated Show resolved Hide resolved

DavidBM added 5 commits February 22, 2020 18:42

finish missing implementations. It compiles!

da9c995

before rebase

create test for async_std

d2d3c23

Merge branch 'combine-4' into async_std

5b2bf7c

add feature flag async-std-aio

22b6587

fix windows build fix missing config attributes for aio

rename function avoiding double async word

d72f8d6

DavidBM and others added 11 commits February 22, 2020 18:42

abstract connection un trait

bcedffc

fix tests

add logic to autodetect reactor

260c54c

simplify cfg attributes for reactor selection

6f0a2ec

Reduce branching in create_connection

75e2505

rename async_std to async_std_dep in order to avoid name collision

35ab78d

fix windows build

Merge pull request #1 from Marwes/async_std

dd78bb8

Reduce branching in create_connection

Make tokio support optional

2d23f6d

- replaces the "aio" default feature by the "tokio-comp" feature. - async-std-comp and tokio-comp features automatically activate aio feature - enabling aio without tokio-comp or async-std-comp will result in a compilation error

add reactor detection for multiplexed connections

ad8193f

fix feature flags names fix windows test format code enable async_std and tokio support by default

Merge branch 'master' into async_std

8caf077

Merge pull request #3 from mitsuhiko/master

5f9b804

sync with master

Code review

0183ee8

Code review

DavidBM force-pushed the async_std branch from a7e4e72 to 0183ee8 Compare February 22, 2020 17:46

badboy reviewed Feb 22, 2020

View reviewed changes

src/aio.rs Outdated Show resolved Hide resolved

hide leaked type

e5cedaf

Marwes approved these changes Feb 26, 2020

View reviewed changes

DavidBM and others added 2 commits March 1, 2020 13:51

Merge branch 'master' into async_std

be538a8

fix: connection manager not able to choose between tokio and async_std

ea24ed8

fix: add feature async_std and tokio flags for connection manager

966d98c

DavidBM force-pushed the async_std branch from 077f719 to 966d98c Compare March 2, 2020 08:22

badboy approved these changes Mar 3, 2020

View reviewed changes

badboy merged commit cc9863b into redis-rs:master Mar 3, 2020

barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Jul 11, 2024

Merge pull request redis-rs#281 from shachlanAmazon/flakey-test

fc1c028

Increase timeout for cluster creation.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async_std support #281

Add async_std support #281

DavidBM commented Feb 5, 2020 •

edited

Loading

DavidBM commented Feb 5, 2020

Marwes left a comment

Marwes Feb 5, 2020

DavidBM Feb 5, 2020

badboy Feb 20, 2020

DavidBM commented Feb 5, 2020

mitsuhiko commented Feb 5, 2020

DavidBM commented Feb 5, 2020

Marwes commented Feb 5, 2020

DavidBM commented Feb 5, 2020

DavidBM commented Feb 5, 2020

DavidBM commented Feb 5, 2020

Marwes commented Feb 6, 2020

DavidBM commented Feb 7, 2020

DavidBM commented Feb 10, 2020

Marwes commented Feb 13, 2020

DavidBM commented Feb 13, 2020

DavidBM commented Feb 13, 2020

DavidBM commented Feb 13, 2020

badboy commented Feb 20, 2020

badboy left a comment

badboy Feb 20, 2020

DavidBM commented Feb 22, 2020

badboy left a comment

DavidBM commented Feb 23, 2020

DavidBM commented Mar 1, 2020

Marwes commented Mar 2, 2020

DavidBM commented Mar 2, 2020

badboy commented Mar 2, 2020

badboy commented Mar 3, 2020

DavidBM commented Mar 3, 2020

Add async_std support #281

Add async_std support #281

Conversation

DavidBM commented Feb 5, 2020 • edited Loading

DavidBM commented Feb 5, 2020

Marwes left a comment

Choose a reason for hiding this comment

Marwes Feb 5, 2020

Choose a reason for hiding this comment

DavidBM Feb 5, 2020

Choose a reason for hiding this comment

badboy Feb 20, 2020

Choose a reason for hiding this comment

DavidBM commented Feb 5, 2020

mitsuhiko commented Feb 5, 2020

DavidBM commented Feb 5, 2020

Marwes commented Feb 5, 2020

DavidBM commented Feb 5, 2020

DavidBM commented Feb 5, 2020

DavidBM commented Feb 5, 2020

Marwes commented Feb 6, 2020

DavidBM commented Feb 7, 2020

DavidBM commented Feb 10, 2020

Marwes commented Feb 13, 2020

DavidBM commented Feb 13, 2020

DavidBM commented Feb 13, 2020

DavidBM commented Feb 13, 2020

badboy commented Feb 20, 2020

badboy left a comment

Choose a reason for hiding this comment

badboy Feb 20, 2020

Choose a reason for hiding this comment

DavidBM commented Feb 22, 2020

badboy left a comment

Choose a reason for hiding this comment

DavidBM commented Feb 23, 2020

DavidBM commented Mar 1, 2020

Marwes commented Mar 2, 2020

DavidBM commented Mar 2, 2020

badboy commented Mar 2, 2020

badboy commented Mar 3, 2020

DavidBM commented Mar 3, 2020

DavidBM commented Feb 5, 2020 •

edited

Loading