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

Integrate async fork #867

Closed
srijs opened this Issue Nov 9, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@srijs
Member

srijs commented Nov 9, 2017

Hi!

@Arcnor and I are currently spending our 20% time at work with Rust, and as part of this we took last week to fork rusoto and convert it to an async API based on futures/tokio.

Following #604 and #714, I would love to work together with you to merge these changes back into this project. Especially since the changes are quite far-reaching, continuing to rebase will be a painful process for me.

The code currently lives here. It fully compiles, and the protocol tests are successful. I have had trouble getting the integration tests to run, which is why I'd be keen to leverage the CI set up in this repo to do it.

The extend of the changes is as follows:

  • core:
    • DispatchSignedRequest is now async (returns a future rather than result)
    • DispatchSignedRequest impl for hyper 0.11
    • introduced RusotoFuture struct which is returned from all service APIs
    • introduced internal Reactor mechanism that provides an implicit event loop to simplify the getting started with rusoto
    • changed default_tls_client to return a DefaultTlsClient struct which uses Reactor internally
  • integration_tests:
    • still synchronous, made compatible by blocking the thread until the future completes
  • mock:
    • converted to futures
  • service_crategen:
    • changed to produce futures-based service code

The story for synchronous use-cases is covered by a .sync() method on the RusotoFuture which will block until a response has been received.

There's a few minor things left to do (documentation mostly) from my side, but I'd like to try and get the ball rolling already if possible.

Also note that the credentials provider is still a blocking API, I'd prefer to get this "mostly async" state upstreamed first, and then follow up with converting CredentialsProvider.

I realise this is a lot and very out-of-the-blue, so apologies in advance for all of the hassle. But please let me know if you'd be interested in proceeding, and maybe we can come up with a good plan!

@SecurityInsanity

This comment has been minimized.

Show comment
Hide comment
@SecurityInsanity

SecurityInsanity Nov 10, 2017

Contributor

👏 I personally would love to see you proceed. Getting rusoto fully async would be awesome.
We'd be happy to help with documentation if you need it, but I'd personally prefer if you pushed it up sooner so we can start reviewing it. As I'm sure it's a pretty big PR.

Contributor

SecurityInsanity commented Nov 10, 2017

👏 I personally would love to see you proceed. Getting rusoto fully async would be awesome.
We'd be happy to help with documentation if you need it, but I'd personally prefer if you pushed it up sooner so we can start reviewing it. As I'm sure it's a pretty big PR.

@adimarco

This comment has been minimized.

Show comment
Hide comment
@adimarco

adimarco Nov 10, 2017

Member

👏 times two. We're definitely psyched to get this in. Async is something that's been our minds for a while, but we just haven't had time to even start given the day jobs we all hold down.

Can you start by getting a PR submitted, even if it's a big one? I'd be glad to dive in and start reviewing and helping. We can actually hold off on other PRs and make this a priority to ease the integration pain.

Member

adimarco commented Nov 10, 2017

👏 times two. We're definitely psyched to get this in. Async is something that's been our minds for a while, but we just haven't had time to even start given the day jobs we all hold down.

Can you start by getting a PR submitted, even if it's a big one? I'd be glad to dive in and start reviewing and helping. We can actually hold off on other PRs and make this a priority to ease the integration pain.

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 28, 2018

Member

We did it!

Member

matthewkmayer commented Jan 28, 2018

We did it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment