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

Move to std::future::Future and Tokio 0.2 to support async/await #1667

Merged
merged 42 commits into from Feb 7, 2020

Conversation

@jen20
Copy link
Contributor

jen20 commented Jan 27, 2020

As discussed in Discord, this is a rebased and consolidated version of all of the work to move Rusoto to Tokio 0.2 and std::future::Future along with async/await support. All of the tests are working on MacOS, though I haven't run the integration tests directly against AWS yet.

A few notes on the process I used to get here:

  • Each crate had changes identified across the relevant pull requests and @iliana's merged branch, and they were reapplied in a single commit for each crate.
  • Authorship has been assigned to the authors who originally did the work, but via Co-authored-by tags on each of the single commits. This turns out primarily to be @dbcfd, @bcmcmill and @iliana.
  • All services were regenerated after addressing some of the issues in the code generator. Generation is now clean.
  • Wherever additional work was required to make more recent contributions compile or behave correctly, I've done so.
  • I've started (but not completed) fixing all lint warnings thrown up by Clippy. Some of these in particular appear to represent real bugs in the core library (particularly replacing for chunk in body.next().await with while let Some(chunk) = body.next().await in rusoto/credential/src/request.rs). Some others were noise and have been disabled via attributes to avoid complicating code unnecessarily to appease the linter.
  • All versions have been updated to 0.50.0 - in order to provide something very visually different from the current version number to verify correctness. This was done in a single commit and can easily be changed to 0.43.0 if this is more desirable.

There are however a few things which are not finished at this point:

  • Some of the custom code in STS has not yet been updated for async and is currently commented out to avoid compilation errors in the meantime.
  • README files have not been updated yet, though documentation tests have
  • Remaining clippy lints need either addressing or silencing.
  • Various new and outdated services are listed by the check function in service_crategen which need addressing.

I hope to look at some more of these over the next day or so.

Proposed CHANGELOG entries

  • Update all futures to std::future::Future, adding support for async and await usage.
  • Upgrade dependencies to Tokio 0.2, Futures 0.3 and associated libraries.
@jen20 jen20 requested a review from iliana Jan 27, 2020
@jen20 jen20 force-pushed the jen20:jen20/rebase-tokio-02 branch from 74df066 to d900064 Jan 27, 2020
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Jan 27, 2020

All versions have been updated to 0.50.0 - in order to provide something very visually different from the current version number to verify correctness. This was done in a single commit and can easily be changed to 0.43.0 if this is more desirable.

Not a fan of "visual difference" in version numbering, especially when semver is enforced.

Let's make it 0.43.0-beta.1, as mentioned in #1663 (comment)

I will hopefully have the time to review this tomorrow :(

@brainstorm

This comment has been minimized.

Copy link

brainstorm commented Jan 28, 2020

So I gather that this PR superseeds #1664 (which perhaps should be closed)? Testing it on the side as this develops :)

@jen20 jen20 force-pushed the jen20:jen20/rebase-tokio-02 branch from d900064 to 2b670b0 Jan 28, 2020
@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Jan 28, 2020

I've updated this to v0.43.0-beta1 for all crates. None of the other items are yet addressed however.

@iliana iliana added this to the 0.43.0 milestone Jan 29, 2020
@iliana iliana self-assigned this Jan 29, 2020
@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Jan 29, 2020

I've taken a look this afternoon at fixing up the various pieces of custom STS code. I believe that the DefaultCredentialsProvider in there may be obsoleted by other work in the credential crate. I am however less clear on the WebIdentity bits, I'll have to do some research to work out what they are for and where they need to go. Since it has dependencies which are likely too broad to add to the credential crate, it may need to stay here and then have a second implementation of DefaultCredentialsProvider which also considers web identity.

@jen20 jen20 force-pushed the jen20:jen20/rebase-tokio-02 branch from 87fee62 to 577622a Feb 1, 2020
@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Feb 2, 2020

I think this is ready to go now. The CI issues do not look easily fixable without better access to the ADO account in which they are running (which I understand to be the original authors?), and GitHub Actions looks like a nonstarter for Windows unless we can self-host runners somehow, since the disk space there is not sufficient.

@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 2, 2020

The CI failure is a missing crate in the workspace Cargo.toml:

error: current package believes it's in a workspace when it's not:
current:   /home/vsts/work/1/s/rusoto/services/forecast/Cargo.toml
workspace: /home/vsts/work/1/s/Cargo.toml

this may be fixable by adding `rusoto/services/forecast` to the `workspace.members` array of the manifest located at: /home/vsts/work/1/s/Cargo.toml

CI has been passing since #1670, so you may need to rebase/merge that in.

Copy link

swarmer left a comment

This isn't a real review, but I tried to have a very passing glance over changes and notice anything that pops out as weird (like missing or unfinished stuff).

rusoto/services/sts/src/custom/default_provider.rs Outdated Show resolved Hide resolved
rusoto/services/sts/src/custom/mod.rs Outdated Show resolved Hide resolved
rusoto/core/src/future.rs Outdated Show resolved Hide resolved
rusoto/core/src/lib.rs Outdated Show resolved Hide resolved
rusoto/credential/src/lib.rs Outdated Show resolved Hide resolved
rusoto/credential/src/lib.rs Show resolved Hide resolved
rusoto/credential/src/object_safe.rs Show resolved Hide resolved
@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Feb 3, 2020

Hi @swarmer! Thanks for looking over this. I'll take a pass at some more cleanup work soon.

@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 4, 2020

(Maintainer note: I'm fine with dropping service crates for this merge as long as we open an issue and treat that as a blocker for 0.43.0. But I don't have a problem with doing a beta release without those crates.)

@jen20

This comment has been minimized.

Copy link
Contributor Author

jen20 commented Feb 4, 2020

@iliana It's worth noting that the dropped crates were all added as part of PR anyway, nothing that was in 0.42.0 is dropped from what I can tell.

jen20 and others added 2 commits Feb 4, 2020
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 4, 2020

Pushed ea7e190 to fix skeptic tests.

Can you answer #1667 (comment)?

I am running integration tests now, and will do a full review this afternoon.

rusoto/core/src/client.rs Outdated Show resolved Hide resolved
@iliana iliana mentioned this pull request Feb 5, 2020
This reapplies changes from 38f3dbf.
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 6, 2020

11225f5 fixes the without_signature integration test.

Next up:

     Running target/debug/deps/elastictranscoder-b8f330228f4a53c3

running 7 tests                                                           
test list_jobs_by_status ... FAILED                                            
test list_pipelines ... FAILED             
test read_preset ... FAILED
test delete_preset ... FAILED                           
test create_preset ... FAILED                      
test list_presets ... FAILED                   
thread panicked while panicking. aborting.      
error: test failed, to rerun pass '--test elastictranscoder'
                                                                               
Caused by:                                           
  process didn't exit successfully: `/home/fedora/rusoto/integration_tests/target/debug/deps/elastictranscoder-b8f330228f4a53c3` (signal: 4, SIGILL: illegal instruction)
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 7, 2020

As of afa3a3a all integration tests pass for me.

We need to look into the Semaphore CI failure; it appears something is now wrong when talking to Ceph.

I'm debating whether I disable the Semaphore tests that now have issues temporarily and build a beta, creating issues to re-enable those as blockers for a final release.

The dot between beta and 1 is load-bearing, just in case we have more
than 9 beta releases.
@iliana iliana force-pushed the jen20:jen20/rebase-tokio-02 branch from 0e0d7c2 to 7f1fe16 Feb 7, 2020
Each test is starting its own Tokio runtime, which I don't expect people
to do in common use (outside of tests).

We're hitting a panic in hyper during the S3 integration tests, and we
need to do a bit of a refactor of the client to get around it, but let's
just run tests slow so we can get a beta out. :)
@iliana iliana force-pushed the jen20:jen20/rebase-tokio-02 branch from 7f1fe16 to ca192de Feb 7, 2020
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 7, 2020

The Ceph (and MinIO) tests were failing for the same reason the S3 tests were locally. The commit message for ca192de explains setting --test-threads 1, and I'll open an issue to track fixing that.

Assuming we win the CI-doesn't-time-out lottery I will merge this :)

@iliana iliana merged commit cf22a43 into rusoto:master Feb 7, 2020
14 of 17 checks passed
14 of 17 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
rusoto.rusoto Build #20200207.4 failed
Details
rusoto.rusoto (Rustls unit tests on OSX (beta channel)) Rustls unit tests on OSX (beta channel) was canceled
Details
rusoto.rusoto (Crate generation on OSX) Crate generation on OSX succeeded
Details
rusoto.rusoto (Crate generation on Windows) Crate generation on Windows succeeded
Details
rusoto.rusoto (Rustls unit tests on Linux (beta channel)) Rustls unit tests on Linux (beta channel) succeeded
Details
rusoto.rusoto (Rustls unit tests on Linux (nightly channel)) Rustls unit tests on Linux (nightly channel) succeeded
Details
rusoto.rusoto (Rustls unit tests on Linux) Rustls unit tests on Linux succeeded
Details
rusoto.rusoto (Rustls unit tests on OSX (nightly channel)) Rustls unit tests on OSX (nightly channel) succeeded
Details
rusoto.rusoto (Skeptic tests on Linux) Skeptic tests on Linux succeeded
Details
rusoto.rusoto (Unit and integration tests on Linux (beta channel)) Unit and integration tests on Linux (beta channel) succeeded
Details
rusoto.rusoto (Unit and integration tests on Linux (nightly channel)) Unit and integration tests on Linux (nightly channel) succeeded
Details
rusoto.rusoto (Unit and integration tests on Linux) Unit and integration tests on Linux succeeded
Details
rusoto.rusoto (Unit and integration tests on OSX (beta channel)) Unit and integration tests on OSX (beta channel) succeeded
Details
rusoto.rusoto (Unit and integration tests on OSX (nightly channel)) Unit and integration tests on OSX (nightly channel) succeeded
Details
rusoto.rusoto (Unit and integration tests on OSX) Unit and integration tests on OSX succeeded
Details
semaphoreci The build passed on Semaphore.
Details
@iliana

This comment has been minimized.

Copy link
Member

iliana commented Feb 7, 2020

Thank you so much @jen20 @dbcfd @bcmcmill!!

@vultix

This comment has been minimized.

Copy link

vultix commented Feb 7, 2020

@iliana Thank you so much for this!! Thank you as well for all of the other work you put into this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.