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

Asynchronous DispatchSignedRequest and Service APIs #868

Merged
merged 3 commits into from Jan 28, 2018

Conversation

Projects
None yet
5 participants
@srijs
Member

srijs commented Nov 10, 2017

Related to #867.

Okay, since @SecurityInsanity and @adimarco have asked me to come forth with a PR, here we go!

I'll spend some time rebasing this evening, in the meantime feel free to start the discussion.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 10, 2017

Member

To make the review a bit more lightweight, and also to simplify the rebase, I've not included the generated files for now.

Once we're all happy with the handwritten portion of the changes, I can invoke the codegen to make the PR complete prior to merging.

Member

srijs commented Nov 10, 2017

To make the review a bit more lightweight, and also to simplify the rebase, I've not included the generated files for now.

Once we're all happy with the handwritten portion of the changes, I can invoke the codegen to make the PR complete prior to merging.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 10, 2017

Member

The builds are running into the travis ci time limit, not sure what the best way would be to fix that... Is this a problem also on master, or just specific to this branch?

Member

srijs commented Nov 10, 2017

The builds are running into the travis ci time limit, not sure what the best way would be to fix that... Is this a problem also on master, or just specific to this branch?

@SecurityInsanity

This comment has been minimized.

Show comment
Hide comment
@SecurityInsanity

SecurityInsanity Nov 10, 2017

Contributor

This problem is unfortunately on master 😭. It's something we have a couple issues open on. I wouldn't worry about it as long as integration tests pass. (Which is something that although Travis doesn't do yet reviewers do).

I'll take a look at this later today 👍

Contributor

SecurityInsanity commented Nov 10, 2017

This problem is unfortunately on master 😭. It's something we have a couple issues open on. I wouldn't worry about it as long as integration tests pass. (Which is something that although Travis doesn't do yet reviewers do).

I'll take a look at this later today 👍

@SecurityInsanity

I just did a quick glance through so there may be some more later on, but I have some questions, and a few nitpicky things.

I'll try to give an in depth review later today

Show outdated Hide outdated integration_tests/tests/acm.rs Outdated
Show outdated Hide outdated rusoto/core/Cargo.toml Outdated
Show outdated Hide outdated rusoto/core/src/future.rs Outdated
Show outdated Hide outdated rusoto/core/src/lib.rs Outdated
Show outdated Hide outdated rusoto/core/src/reactor.rs Outdated
Show outdated Hide outdated rusoto/core/src/request.rs Outdated
Show outdated Hide outdated rusoto/core/src/request.rs Outdated
type Item = HttpResponse;
type Error = HttpDispatchError;
fn poll(&mut self) -> Poll<Self::Item, Self::Error> {

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 10, 2017

Contributor

So I don't see support for timeouts here. This is something that I'd really love to have (and we do it in the credentials crate). Am I missing it, or is it not a part of it?

@SecurityInsanity

SecurityInsanity Nov 10, 2017

Contributor

So I don't see support for timeouts here. This is something that I'd really love to have (and we do it in the credentials crate). Am I missing it, or is it not a part of it?

This comment has been minimized.

@srijs

srijs Nov 11, 2017

Member

Nope, it's not in here yet. To me this seems like something we could ship after the initial async changes, since a) we can very likely add it later w/o breaking the API and b) it's not part of the current rusoto feature set either. What do you think?

@srijs

srijs Nov 11, 2017

Member

Nope, it's not in here yet. To me this seems like something we could ship after the initial async changes, since a) we can very likely add it later w/o breaking the API and b) it's not part of the current rusoto feature set either. What do you think?

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

I think we'd want at least some basic support for timeouts. I know it's not part of the feature set, but I could very well see someone using the async provider, and wondered why certain code isn't getting called. That extra layer of indirection I think makes it harder to easily spot "oh it's a timeout"

Plus personally I think it's much easier to support timeouts with futures.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

I think we'd want at least some basic support for timeouts. I know it's not part of the feature set, but I could very well see someone using the async provider, and wondered why certain code isn't getting called. That extra layer of indirection I think makes it harder to easily spot "oh it's a timeout"

Plus personally I think it's much easier to support timeouts with futures.

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Per parent comments I agree we want timeouts, but if adding them to this PR makes it a lot larger it's harder to get the changes into master. I'm okay with leaving them out of this PR and getting them in as soon as this one lands. 😄

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Per parent comments I agree we want timeouts, but if adding them to this PR makes it a lot larger it's harder to get the changes into master. I'm okay with leaving them out of this PR and getting them in as soon as this one lands. 😄

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

Adding timeouts shouldn't add too much to this commit which is why I suggest adding it in this one (in previous it was a couple lines of error handling). Tokio can natively support this by joining two futures together (this is what the creds crate does now).

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

Adding timeouts shouldn't add too much to this commit which is why I suggest adding it in this one (in previous it was a couple lines of error handling). Tokio can natively support this by joining two futures together (this is what the creds crate does now).

Show outdated Hide outdated rusoto/services/sts/src/custom/credential.rs Outdated
Show outdated Hide outdated service_crategen/src/commands/generate/codegen/mod.rs Outdated
@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 11, 2017

Member

Thanks for jumping on this so quickly, @SecurityInsanity. Addressed your feedback so far.

Looking forward to the in-depth review!

Member

srijs commented Nov 11, 2017

Thanks for jumping on this so quickly, @SecurityInsanity. Addressed your feedback so far.

Looking forward to the in-depth review!

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Nov 11, 2017

Member

@SecurityInsanity and @adimarco can take care of business while I'm booked up doing other tasks for a bit. 😄

I'm quite looking forward to getting this in, thanks all for their hard work! 👍

Member

matthewkmayer commented Nov 11, 2017

@SecurityInsanity and @adimarco can take care of business while I'm booked up doing other tasks for a bit. 😄

I'm quite looking forward to getting this in, thanks all for their hard work! 👍

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 16, 2017

Member

Just a quick update here: I've been able to make good progress on the async ProvideAwsCredentials piece of work, and hope that I can wrap it up over the weekend. Will update this PR when I do.

@SecurityInsanity and @adimarco: If you managed to set some time aside early next week for a second round of reviews, that would be hugely appreciated!

Member

srijs commented Nov 16, 2017

Just a quick update here: I've been able to make good progress on the async ProvideAwsCredentials piece of work, and hope that I can wrap it up over the weekend. Will update this PR when I do.

@SecurityInsanity and @adimarco: If you managed to set some time aside early next week for a second round of reviews, that would be hugely appreciated!

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 17, 2017

Member

PR is now updated with my changes to make ProvideAwsCredentials async.

Member

srijs commented Nov 17, 2017

PR is now updated with my changes to make ProvideAwsCredentials async.

@SecurityInsanity

After reviewing this I have to say this not only improves overall upon my futures work in credentials (which was very very clunky).

That being said before we start on the generation, and asking people to test it before release I have one apprehension (which isn't a blocker) the fact that our service crates don't provide timeouts. I know this isn't currently in which is why I think it's probably fine, but I'd still really like it.

The blocker for me is the removal from timeout support in the credentials crate. We really don't want to remove something.

Show outdated Hide outdated .travis.yml Outdated
extern crate url;
extern crate xml;
mod future;

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

If we're pub use future::RusotoFuture why not make this a: pub mod as well?

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

If we're pub use future::RusotoFuture why not make this a: pub mod as well?

This comment has been minimized.

@srijs

srijs Nov 17, 2017

Member

I didn't make it a pub mod, since the only thing it exports is RusotoFuture, which is already exported on the top level. Happy to expose it if you want me to.

@srijs

srijs Nov 17, 2017

Member

I didn't make it a pub mod, since the only thing it exports is RusotoFuture, which is already exported on the top level. Happy to expose it if you want me to.

type Item = HttpResponse;
type Error = HttpDispatchError;
fn poll(&mut self) -> Poll<Self::Item, Self::Error> {

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

I think we'd want at least some basic support for timeouts. I know it's not part of the feature set, but I could very well see someone using the async provider, and wondered why certain code isn't getting called. That extra layer of indirection I think makes it harder to easily spot "oh it's a timeout"

Plus personally I think it's much easier to support timeouts with futures.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

I think we'd want at least some basic support for timeouts. I know it's not part of the feature set, but I could very well see someone using the async provider, and wondered why certain code isn't getting called. That extra layer of indirection I think makes it harder to easily spot "oh it's a timeout"

Plus personally I think it's much easier to support timeouts with futures.

use {AwsCredentials, CredentialsError, ProvideAwsCredentials, ProvideTimeoutableAwsCredentials,
use {AwsCredentials, CredentialsError, ProvideAwsCredentials,

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

So I'm all for redoing my initial async credentials crate work (because honestly it was pretty clunky, and just to see a very basic implementation). But the way I'm reading this we're actually losing timeouts with credentials? If so I know we don't want to do that, as several people I work with (as well as I'm sure others) have already built ontop of this functionality since it's now released.

@SecurityInsanity

SecurityInsanity Nov 17, 2017

Contributor

So I'm all for redoing my initial async credentials crate work (because honestly it was pretty clunky, and just to see a very basic implementation). But the way I'm reading this we're actually losing timeouts with credentials? If so I know we don't want to do that, as several people I work with (as well as I'm sure others) have already built ontop of this functionality since it's now released.

This comment has been minimized.

@srijs

srijs Nov 17, 2017

Member

Apologies, I didn't mean to imply that I want to get rid of the timeout mechanism, I was merely trying to get the basic async structure right first.

I'm open to either initiate the discussion around how to add timeouts as part of this PR, or to commit to implementing timeouts in a second PR before the next rusoto release under your guidance.

Personally I would prefer to get the basic async changes merged first, and then follow up with a second PR before we release the new changes (or even ask people to test pre-release), but I am willing to go whichever route you think makes more sense.

Let me know which one you'd prefer!

@srijs

srijs Nov 17, 2017

Member

Apologies, I didn't mean to imply that I want to get rid of the timeout mechanism, I was merely trying to get the basic async structure right first.

I'm open to either initiate the discussion around how to add timeouts as part of this PR, or to commit to implementing timeouts in a second PR before the next rusoto release under your guidance.

Personally I would prefer to get the basic async changes merged first, and then follow up with a second PR before we release the new changes (or even ask people to test pre-release), but I am willing to go whichever route you think makes more sense.

Let me know which one you'd prefer!

Show outdated Hide outdated service_crategen/src/main.rs Outdated
@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Nov 19, 2017

Member

I'm fine with adding the --no-format behavior to the crategen project. Formatting does take a long time and we still haven't tackled CI failing builds if the checked-in code doesn't have rustfmt applied. We don't really lose anything by doing this at the moment.

While I'd prefer not to remove the timeout behavior from the credentials crate, I think adding more to this PR may not be the easiest way forward: Rusoto maintainer time is limited right now, so smaller changes are easier to get in. If there's an immediate followup PR to get that back in, everything would be great.

I'm still working through the changes in the PR. Futures and Tokio are something I've been unable to spend much time working through so there's lots of reading on my end. 😄

Member

matthewkmayer commented Nov 19, 2017

I'm fine with adding the --no-format behavior to the crategen project. Formatting does take a long time and we still haven't tackled CI failing builds if the checked-in code doesn't have rustfmt applied. We don't really lose anything by doing this at the moment.

While I'd prefer not to remove the timeout behavior from the credentials crate, I think adding more to this PR may not be the easiest way forward: Rusoto maintainer time is limited right now, so smaller changes are easier to get in. If there's an immediate followup PR to get that back in, everything would be great.

I'm still working through the changes in the PR. Futures and Tokio are something I've been unable to spend much time working through so there's lots of reading on my end. 😄

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 20, 2017

Member

Thanks, @matthewkmayer!

As I said earlier, I'm happy to commit to following up with another (smaller) PR to reinstate the timeout functionality.

I'll be standing by until further feedback. I appreciate all your hard work in maintaining rusoto and taking the time to review PRs such as this one!

Member

srijs commented Nov 20, 2017

Thanks, @matthewkmayer!

As I said earlier, I'm happy to commit to following up with another (smaller) PR to reinstate the timeout functionality.

I'll be standing by until further feedback. I appreciate all your hard work in maintaining rusoto and taking the time to review PRs such as this one!

};
lazy_static! {
static ref DEFAULT_REACTOR: Reactor = {

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Is having a single Reactor a standard pattern? My understanding of event loops says yes but I'd like to make sure. 😄

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Is having a single Reactor a standard pattern? My understanding of event loops says yes but I'd like to make sure. 😄

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

I've seen it this way in other languages, but I have to say I don't know if it is for tokio. I'd also be interested in knowing this.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

I've seen it this way in other languages, but I have to say I don't know if it is for tokio. I'd also be interested in knowing this.

This comment has been minimized.

@srijs

srijs Nov 26, 2017

Member

So, answering this requires going into a bit of context, so here it goes :)

One of my main goals when approaching this feature was to make rusoto still usable in a non-async codebase without a lot of extra ceremony required: Effectively the migration path to the new rusoto version should be as easy as possible for existing consumers.

In order to achieve that, I added the RusotoFuture::sync method as a simple way of blocking the current thread until a response arrives. However, calling Futures::wait (which ::sync does internally) is not enough to make it work. Without an event loop in the background driving the I/O, ::sync would just block forever.

While a single "default" event loop is not a standard pattern for async Rust at the moment (the hyper client for instance needs to be instantiated by referencing an existing event loop), and we should still support the more "advanced" use-cases (which this PR does), I believe introducing a default event loop here is a good idea:

Tokio is currently planning a reform of the tokio crates, with a very similar goal, to simplify the integration story for the majority of tokio users. You can read more what they are planning here. As part of this effort, they are going to introduce a default event loop, very similar to what the reactor is providing here.

Unfortunately, the tokio reform still seems to be multiple months (~6?) out at this point. So while we can't take advantage of the new tokio default event loop just yet, the way the default loop is implemented in this PR will allow us to migrate to the new tokio pattern as soon as it is released, without any breaking changes.

So tl;dr: There is no standard pattern for this yet, however tokio is moving into this direction and the implementation in this PR aligns us well with where tokio is headed.

@srijs

srijs Nov 26, 2017

Member

So, answering this requires going into a bit of context, so here it goes :)

One of my main goals when approaching this feature was to make rusoto still usable in a non-async codebase without a lot of extra ceremony required: Effectively the migration path to the new rusoto version should be as easy as possible for existing consumers.

In order to achieve that, I added the RusotoFuture::sync method as a simple way of blocking the current thread until a response arrives. However, calling Futures::wait (which ::sync does internally) is not enough to make it work. Without an event loop in the background driving the I/O, ::sync would just block forever.

While a single "default" event loop is not a standard pattern for async Rust at the moment (the hyper client for instance needs to be instantiated by referencing an existing event loop), and we should still support the more "advanced" use-cases (which this PR does), I believe introducing a default event loop here is a good idea:

Tokio is currently planning a reform of the tokio crates, with a very similar goal, to simplify the integration story for the majority of tokio users. You can read more what they are planning here. As part of this effort, they are going to introduce a default event loop, very similar to what the reactor is providing here.

Unfortunately, the tokio reform still seems to be multiple months (~6?) out at this point. So while we can't take advantage of the new tokio default event loop just yet, the way the default loop is implemented in this PR will allow us to migrate to the new tokio pattern as soon as it is released, without any breaking changes.

So tl;dr: There is no standard pattern for this yet, however tokio is moving into this direction and the implementation in this PR aligns us well with where tokio is headed.

@@ -40,7 +41,7 @@ pub struct HttpResponse {
/// Status code of HTTP Request
pub status: StatusCode,
/// Contents of Response
pub body: Box<Read>,
pub body: Box<Stream<Item=Vec<u8>, Error=HttpDispatchError> + Send>,

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

I'm not clear on the behavior here: does the redefinition of body mean it will use a Vec<u8> internally? We purposefully hid the contents behind the Read trait to allow streaming reads from services. An example is downloading a multi-GB file from S3: we don't want to load the whole thing into memory.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

I'm not clear on the behavior here: does the redefinition of body mean it will use a Vec<u8> internally? We purposefully hid the contents behind the Read trait to allow streaming reads from services. An example is downloading a multi-GB file from S3: we don't want to load the whole thing into memory.

This comment has been minimized.

@srijs

srijs Nov 26, 2017

Member

Item=Vec<u8> here means that the stream delivers "chunks" of data in the form of Vec<u8>. Think of this as similar to an Iterator<Item=Vec<u8>>, except asynchronous.

@srijs

srijs Nov 26, 2017

Member

Item=Vec<u8> here means that the stream delivers "chunks" of data in the form of Vec<u8>. Think of this as similar to an Iterator<Item=Vec<u8>>, except asynchronous.

/// Dispatch Request, and then return a Response
fn dispatch(&self, request: &SignedRequest) -> Result<HttpResponse, HttpDispatchError>;
fn dispatch(&self, request: SignedRequest) -> Self::Future;

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Do we no longer take a reference to a SignedRequest because the underlying object is being moved to the reactor thread?

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Do we no longer take a reference to a SignedRequest because the underlying object is being moved to the reactor thread?

This comment has been minimized.

@srijs

srijs Nov 26, 2017

Member

Yep, that's basically right! Generally borrowing does not work well with futures, and we often want to move the arguments to a function into the future in order to use them there.

It can also help us later to eliminate some of the copying that is currently happening when transforming SignedRequest into a hyper::Request, so I think it's a good thing to do.

@srijs

srijs Nov 26, 2017

Member

Yep, that's basically right! Generally borrowing does not work well with futures, and we often want to move the arguments to a function into the future in order to use them there.

It can also help us later to eliminate some of the copying that is currently happening when transforming SignedRequest into a hyper::Request, so I think it's a good thing to do.

let ssl = match NativeTlsClient::new() {
Ok(ssl) => ssl,
pub fn default_tls_client(handle: &Handle) -> Result<Client<HttpsConnector<HttpConnector>>, TlsError> {
let connector = match HttpsConnector::new(4, handle) {

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Is this making a new HTTPS connection pool of size 4?

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Is this making a new HTTPS connection pool of size 4?

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

This is actually the number of DNS Resolver threads: https://dabo.guru/rust/screeps-api/hyper_tls/struct.HttpsConnector.html

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

This is actually the number of DNS Resolver threads: https://dabo.guru/rust/screeps-api/hyper_tls/struct.HttpsConnector.html

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Ah, thanks, good to know. Ensuring we keep the connection pool feature is important. Does hyper async (0.11 and later) do that for us?

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

Ah, thanks, good to know. Ensuring we keep the connection pool feature is important. Does hyper async (0.11 and later) do that for us?

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

Not sure actually. That's a good catch. I don't see anything in changelog about it. Would be worth testing though.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

Not sure actually. That's a good catch. I don't see anything in changelog about it. Would be worth testing though.

This comment has been minimized.

@srijs

srijs Nov 26, 2017

Member

Hyper >=0.11 still maintains a connection pool, no regression here ;)

@srijs

srijs Nov 26, 2017

Member

Hyper >=0.11 still maintains a connection pool, no regression here ;)

let instance_metadata_provider = self.instance_metadata_provider.clone();
let container_provider = self.container_provider.clone();
let future = EnvironmentProvider.credentials()
.or_else(move |_| match profile_provider {

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

The or_else chain is how we do the prioritized list of credential sourcing in this PR, right?

@matthewkmayer

matthewkmayer Nov 22, 2017

Member

The or_else chain is how we do the prioritized list of credential sourcing in this PR, right?

This comment has been minimized.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

This is existing behavior, and yes this is how we choose which sources to check first.

@SecurityInsanity

SecurityInsanity Nov 22, 2017

Contributor

This is existing behavior, and yes this is how we choose which sources to check first.

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Nov 22, 2017

Member

I've commented on sections where my lack of Tokio and Futures knowledge shows so I can better understand these changes. 😄

Other notes after generating crate code from this PR changeset:

  • rustfmt fails with trailing whitespace a lot for some reason. Be nice to find and fix those problems.
  • crategen is really really slow with this PR, even when compiling with --release.
  • Compilation time appears to have significantly regressed with this PR: in the root directory I ran cargo clean && cargo build --all and it took 500 seconds on my low-end macbook (average of two runs). For comparison, master takes 435 seconds (average of two runs). Related to rust-lang/rust#38528 ?
  • lots of unused variable warnings in the generated code. Hopefully easy changes to fix in code generation.

(edit) I used the --no-format flag since we apparently hit some really bad performance use case of the old version of rustfmt we use and crate generation is about as fast as expected.

Member

matthewkmayer commented Nov 22, 2017

I've commented on sections where my lack of Tokio and Futures knowledge shows so I can better understand these changes. 😄

Other notes after generating crate code from this PR changeset:

  • rustfmt fails with trailing whitespace a lot for some reason. Be nice to find and fix those problems.
  • crategen is really really slow with this PR, even when compiling with --release.
  • Compilation time appears to have significantly regressed with this PR: in the root directory I ran cargo clean && cargo build --all and it took 500 seconds on my low-end macbook (average of two runs). For comparison, master takes 435 seconds (average of two runs). Related to rust-lang/rust#38528 ?
  • lots of unused variable warnings in the generated code. Hopefully easy changes to fix in code generation.

(edit) I used the --no-format flag since we apparently hit some really bad performance use case of the old version of rustfmt we use and crate generation is about as fast as expected.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 26, 2017

Member

@matthewkmayer Addressed most of your in-line feedback, will go ahead and rebase this one now...

It seems like your points can be mostly clustered into two categories:

  • Crategen and compilation time: I'm not 100% sure what I can do to improve that. From experience, moving from a template+format approach to an AST-based one often helps to get generation times under control. With regards to compilation time, maybe it is worth seeing which parts of the generated code could be properly abstracted and moved into a separate reusable crate, with the goal to reduce the total number of lines that need to be compiled.

  • Formatting and linting: I can spend some time seeing if there's any low-hanging fruit. Do you have any idea what makes rustfmt fail? Since the code is syntactically valid, it is not clear to me how I could address the rustfmt failures.

Looking forward to hearing from you!

Member

srijs commented Nov 26, 2017

@matthewkmayer Addressed most of your in-line feedback, will go ahead and rebase this one now...

It seems like your points can be mostly clustered into two categories:

  • Crategen and compilation time: I'm not 100% sure what I can do to improve that. From experience, moving from a template+format approach to an AST-based one often helps to get generation times under control. With regards to compilation time, maybe it is worth seeing which parts of the generated code could be properly abstracted and moved into a separate reusable crate, with the goal to reduce the total number of lines that need to be compiled.

  • Formatting and linting: I can spend some time seeing if there's any low-hanging fruit. Do you have any idea what makes rustfmt fail? Since the code is syntactically valid, it is not clear to me how I could address the rustfmt failures.

Looking forward to hearing from you!

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Nov 26, 2017

Member

Okay, so I managed to reduce the number of rustfmt errors quite a bit by updating to rustfmt 0.9.0, as well as reducing the depth of the generated syntax tree.

At this point only xml-based APIs are still failing with rustfmt, I'll keep working on this.

Member

srijs commented Nov 26, 2017

Okay, so I managed to reduce the number of rustfmt errors quite a bit by updating to rustfmt 0.9.0, as well as reducing the depth of the generated syntax tree.

At this point only xml-based APIs are still failing with rustfmt, I'll keep working on this.

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Dec 3, 2017

Member

Thanks! I'm back from my trip and will be looking at this in the next few days.

Member

matthewkmayer commented Dec 3, 2017

Thanks! I'm back from my trip and will be looking at this in the next few days.

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Dec 5, 2017

Member

#870 is a big bug, so I'll be tackling that before getting back to this PR. Thanks for your patience!

Member

matthewkmayer commented Dec 5, 2017

#870 is a big bug, so I'll be tackling that before getting back to this PR. Thanks for your patience!

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Dec 6, 2017

Member

@matthewkmayer thanks for the heads-up!

As a quick update, I've just pushed some more changes that now completely eliminate the rustfmt errors. It appears syntax tree depth was really the key here.

While waiting for your feedback, I'll continue to see if there's an easy way to fix up the unused variable warnings.

Member

srijs commented Dec 6, 2017

@matthewkmayer thanks for the heads-up!

As a quick update, I've just pushed some more changes that now completely eliminate the rustfmt errors. It appears syntax tree depth was really the key here.

While waiting for your feedback, I'll continue to see if there's an easy way to fix up the unused variable warnings.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Dec 6, 2017

Member

Good news! Just pushed an update to the branch that should fix up all unused variable warnings!

Member

srijs commented Dec 6, 2017

Good news! Just pushed an update to the branch that should fix up all unused variable warnings!

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Dec 20, 2017

Member

I've not forgotten this PR! 😄 In fact I've run across an issue with connection pooling with hyper 0.10.x that is specifically won't fix in those version. hyper 0.11.x has the fix so getting async hyper is important.

Member

matthewkmayer commented Dec 20, 2017

I've not forgotten this PR! 😄 In fact I've run across an issue with connection pooling with hyper 0.10.x that is specifically won't fix in those version. hyper 0.11.x has the fix so getting async hyper is important.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Dec 28, 2017

Member

Quick update here: I just pushed another commit that speeds up compilation time, to be more or less on-par with master.

The current state is now that this PR generates 763687 lines of code for the services, which is an increase of 6.6% compared to master. Compilation on my machine takes about 3m35s (avg of two runs), where master takes 3m26s (avg of two runs), which is a 4.3% increase and so more or less in relation with the additional lines of code.

The biggest problem is still the codegen, which with formatting enabled takes about 12 minutes on my machine (non-release build), while on master it’s about 30s. That’s a significant regression. As you said earlier, it appears that we’re running into some bad cases in rustfmt. I’ll spend some time over the next days digging into rustfmt and seeing if I can identify what exactly the problems are.

Member

srijs commented Dec 28, 2017

Quick update here: I just pushed another commit that speeds up compilation time, to be more or less on-par with master.

The current state is now that this PR generates 763687 lines of code for the services, which is an increase of 6.6% compared to master. Compilation on my machine takes about 3m35s (avg of two runs), where master takes 3m26s (avg of two runs), which is a 4.3% increase and so more or less in relation with the additional lines of code.

The biggest problem is still the codegen, which with formatting enabled takes about 12 minutes on my machine (non-release build), while on master it’s about 30s. That’s a significant regression. As you said earlier, it appears that we’re running into some bad cases in rustfmt. I’ll spend some time over the next days digging into rustfmt and seeing if I can identify what exactly the problems are.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Dec 28, 2017

Member

More good news! Rebasing this branch on top of #897 brings codegen times back to under 30 seconds!

In light of this I would advocate to focus on merging #897 first.

Member

srijs commented Dec 28, 2017

More good news! Rebasing this branch on top of #897 brings codegen times back to under 30 seconds!

In light of this I would advocate to focus on merging #897 first.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 5, 2018

Member

Rebased and split into multiple commits! Travis timed out again, will need a re-run.

Member

srijs commented Jan 5, 2018

Rebased and split into multiple commits! Travis timed out again, will need a re-run.

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 6, 2018

Member

While the TravisCI failure is due to timeouts and not issues with this PR, the Appveyor builds do appear to have something off with this PR. Anything I can give a hand with for sorting that out?

Member

matthewkmayer commented Jan 6, 2018

While the TravisCI failure is due to timeouts and not issues with this PR, the Appveyor builds do appear to have something off with this PR. Anything I can give a hand with for sorting that out?

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 6, 2018

Member

@matthewkmayer the Appveyor failure is likely due to the fact that I didn't check in the generated code yet (and the CI script for Appveyor doesn't perform generation itself).

I'll check in the generated code shortly to make the build green, and will also attempt to run the integration tests locally.

Member

srijs commented Jan 6, 2018

@matthewkmayer the Appveyor failure is likely due to the fact that I didn't check in the generated code yet (and the CI script for Appveyor doesn't perform generation itself).

I'll check in the generated code shortly to make the build green, and will also attempt to run the integration tests locally.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 7, 2018

Member

Okay, successfully pushed the generated code to the branch:

  • 33188b0 contains the base changes
  • 525a547 contains changes to the integration tests
  • 64777cd contains changes from the codegen

Appveyor build is now green, however a single Travis build timed out again :/

I also ran the integration tests and they were successful!

Member

srijs commented Jan 7, 2018

Okay, successfully pushed the generated code to the branch:

  • 33188b0 contains the base changes
  • 525a547 contains changes to the integration tests
  • 64777cd contains changes from the codegen

Appveyor build is now green, however a single Travis build timed out again :/

I also ran the integration tests and they were successful!

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 7, 2018

Member

Things left before merging:

  • another review of the code by a maintainer
  • maintainer running integration tests

Close! 👍

Member

matthewkmayer commented Jan 7, 2018

Things left before merging:

  • another review of the code by a maintainer
  • maintainer running integration tests

Close! 👍

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 10, 2018

Member

We might also want to consider flushing out some of the other currently open PRs and cutting a release before merging this.

Member

srijs commented Jan 10, 2018

We might also want to consider flushing out some of the other currently open PRs and cutting a release before merging this.

@xrl

This comment has been minimized.

Show comment
Hide comment
@xrl

xrl Jan 10, 2018

Contributor

I hit a throughput limitation (and some instability) when using the Kinesis API with the current stable Rusoto. I have switched to this branch of the code and I am hitting some errors with the reactor failing during requests under concurrent writes (even 2 concurrent writes). I'm flushing about 500k of data out per request.

CPU load is not high, each thread has about 10% of one CPU (16 core box). The tokio reactor thread has about 10% of one CPU. I guessed what thread was doing what based on the PIDs.

I filed a bug with the hyper project: hyperium/hyper#1413 . But I'll copy the error messages here:

thread '<unnamed>' panicked at 'assertion failed: !self.can_read_head() && !self.can_read_body()', /root/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.11.11/src/proto/conn.rs:262:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread '<unnamed>' panicked at 'failed to retrieve response from reactor: Canceled', libcore/result.rs:916:5
### And a bit later
thread '<unnamed>' panicked at 'failed to send request to reactor: send failed because receiver is gone', /root/.cargo/git/checkouts/rusoto-async-35a81945dd339d5f/64777cd/rusoto/core/src/reactor.rs:151:13

You can see my Kinesis client code here: https://github.com/tureus/log-sloth/blob/master/src/bin/log-sloth.rs#L249-L296

Contributor

xrl commented Jan 10, 2018

I hit a throughput limitation (and some instability) when using the Kinesis API with the current stable Rusoto. I have switched to this branch of the code and I am hitting some errors with the reactor failing during requests under concurrent writes (even 2 concurrent writes). I'm flushing about 500k of data out per request.

CPU load is not high, each thread has about 10% of one CPU (16 core box). The tokio reactor thread has about 10% of one CPU. I guessed what thread was doing what based on the PIDs.

I filed a bug with the hyper project: hyperium/hyper#1413 . But I'll copy the error messages here:

thread '<unnamed>' panicked at 'assertion failed: !self.can_read_head() && !self.can_read_body()', /root/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.11.11/src/proto/conn.rs:262:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread '<unnamed>' panicked at 'failed to retrieve response from reactor: Canceled', libcore/result.rs:916:5
### And a bit later
thread '<unnamed>' panicked at 'failed to send request to reactor: send failed because receiver is gone', /root/.cargo/git/checkouts/rusoto-async-35a81945dd339d5f/64777cd/rusoto/core/src/reactor.rs:151:13

You can see my Kinesis client code here: https://github.com/tureus/log-sloth/blob/master/src/bin/log-sloth.rs#L249-L296

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 11, 2018

Member

@xrl thanks, I’ll look into that this evening!

Member

srijs commented Jan 11, 2018

@xrl thanks, I’ll look into that this evening!

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 11, 2018

Member

We might also want to consider flushing out some of the other currently open PRs and cutting a release before merging this.

Agreed. If we knock out a few PRs and release 0.31.0 we can shake the bugs out of this PR and release 0.32.0 shortly afterwards. 😄

Member

matthewkmayer commented Jan 11, 2018

We might also want to consider flushing out some of the other currently open PRs and cutting a release before merging this.

Agreed. If we knock out a few PRs and release 0.31.0 we can shake the bugs out of this PR and release 0.32.0 shortly afterwards. 😄

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 11, 2018

Member

Commented on the issue in hyper, it indeed looks like all of this is caused by hyper panicking for some reason.

Member

srijs commented Jan 11, 2018

Commented on the issue in hyper, it indeed looks like all of this is caused by hyper panicking for some reason.

@xrl

This comment has been minimized.

Show comment
Hide comment
@xrl

xrl Jan 12, 2018

Contributor

OK, turns out I had a bad VPN for my AWS VPC. The issue has gone away and I'm back to stable Rusoto use.

Now I have a question about the DEFAULT_REACTOR's visibility. I think it should be public so I can do some more fancy work. I'm trying to make things go FASTER. I want to set up a deep pipeline of Kinesis PutRecords requests but I'm having a hard time. Here's the code I'm trying:

            recs.push(PutRecordsRequestEntry {
                data: json_vecu8,
                explicit_hash_key: None,
                partition_key: partition_key,
            });

            if recs.len() >= capacity {
                info!("pushing...");
                queue.push(RusotoFuture::new(kinesis_client
                    .put_records(&PutRecordsInput {
                        records: recs.clone(),
                        stream_name: stream_name.clone(),
                    }).and_then(|pro| {
                        info!("hi!");
                        futures::done(Ok(pro))
                    })
                ));
                recs.clear();
                DEFAULT_REACTOR.remote.run(||{
                    match queue.poll() {
                        Ok(thing) => {
                            info!("got something {:?}", thing);
                        },
                        Err(e) => {
                            info!("drat, came up err {:?}", e);
                        }
                    }
                })
            }

but that won't work. Is my code a bad idea or should we make DEFAULT_REACTOR and its "inner" Reactor handle public?

Contributor

xrl commented Jan 12, 2018

OK, turns out I had a bad VPN for my AWS VPC. The issue has gone away and I'm back to stable Rusoto use.

Now I have a question about the DEFAULT_REACTOR's visibility. I think it should be public so I can do some more fancy work. I'm trying to make things go FASTER. I want to set up a deep pipeline of Kinesis PutRecords requests but I'm having a hard time. Here's the code I'm trying:

            recs.push(PutRecordsRequestEntry {
                data: json_vecu8,
                explicit_hash_key: None,
                partition_key: partition_key,
            });

            if recs.len() >= capacity {
                info!("pushing...");
                queue.push(RusotoFuture::new(kinesis_client
                    .put_records(&PutRecordsInput {
                        records: recs.clone(),
                        stream_name: stream_name.clone(),
                    }).and_then(|pro| {
                        info!("hi!");
                        futures::done(Ok(pro))
                    })
                ));
                recs.clear();
                DEFAULT_REACTOR.remote.run(||{
                    match queue.poll() {
                        Ok(thing) => {
                            info!("got something {:?}", thing);
                        },
                        Err(e) => {
                            info!("drat, came up err {:?}", e);
                        }
                    }
                })
            }

but that won't work. Is my code a bad idea or should we make DEFAULT_REACTOR and its "inner" Reactor handle public?

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 12, 2018

Member

@xrl The default reactor at this point was primarily meant to ease the integration of Rusoto in synchronous code-bases (the .sync() method requires a background event loop).

We could expose it directly, however then I feel we'd be painting ourselves into a corner a bit with regards to the migration path as soon as the tokio reform changes land.

However, seems to me like you don't necessarily need access to the reactor to set up that pipeline. Have you considered using futures::sync::mpsc as the mechanism of sending records from the threads listening on the incoming tcp connections to a shared sender thread? futures::sync::mpsc::Receiver implements the Stream trait, so you could use the Stream::chunks method to batch up to 500 records, and then use Stream::for_each to submit each batch to Kinesis.

Member

srijs commented Jan 12, 2018

@xrl The default reactor at this point was primarily meant to ease the integration of Rusoto in synchronous code-bases (the .sync() method requires a background event loop).

We could expose it directly, however then I feel we'd be painting ourselves into a corner a bit with regards to the migration path as soon as the tokio reform changes land.

However, seems to me like you don't necessarily need access to the reactor to set up that pipeline. Have you considered using futures::sync::mpsc as the mechanism of sending records from the threads listening on the incoming tcp connections to a shared sender thread? futures::sync::mpsc::Receiver implements the Stream trait, so you could use the Stream::chunks method to batch up to 500 records, and then use Stream::for_each to submit each batch to Kinesis.

@xrl

This comment has been minimized.

Show comment
Hide comment
@xrl

xrl Jan 13, 2018

Contributor

However, seems to me like you don't necessarily need access to the reactor to set up that pipeline.. I think I do, the tx/rx pair has to be used inside of the reactor in order to pull out futures. A useful function to send futures in the reactor would be the (futures::sync::mpsc::spawn)[http://alexcrichton.com/futures-rs/futures/sync/mpsc/fn.spawn.html]. The spawn signature requires an E: Executor<Execute<S>> (aka, the remote property of the rusoto_core::reactor::Reactor).

Contributor

xrl commented Jan 13, 2018

However, seems to me like you don't necessarily need access to the reactor to set up that pipeline.. I think I do, the tx/rx pair has to be used inside of the reactor in order to pull out futures. A useful function to send futures in the reactor would be the (futures::sync::mpsc::spawn)[http://alexcrichton.com/futures-rs/futures/sync/mpsc/fn.spawn.html]. The spawn signature requires an E: Executor<Execute<S>> (aka, the remote property of the rusoto_core::reactor::Reactor).

@xrl

This comment has been minimized.

Show comment
Hide comment
@xrl

xrl Jan 15, 2018

Contributor

I got a super hacky solution, using a bounded queue and spawning workers to call sync() on the futures: https://gist.github.com/xrl/5b715cd5b8c114314d7e6b52ecf36d8b

Contributor

xrl commented Jan 15, 2018

I got a super hacky solution, using a bounded queue and spawning workers to call sync() on the futures: https://gist.github.com/xrl/5b715cd5b8c114314d7e6b52ecf36d8b

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 15, 2018

Member

@xrl Let‘s move this discussion out of this PR please. I‘ll comment on your gist later with some suggestions!

Member

srijs commented Jan 15, 2018

@xrl Let‘s move this discussion out of this PR please. I‘ll comment on your gist later with some suggestions!

@luben

This comment has been minimized.

Show comment
Hide comment
@luben

luben Jan 17, 2018

Regarding timeouts, one approach is to add the timeout as argument to sync, e.g. .sync(ms). In the async case I think it should be left to the consumer to define and handle them.

luben commented Jan 17, 2018

Regarding timeouts, one approach is to add the timeout as argument to sync, e.g. .sync(ms). In the async case I think it should be left to the consumer to define and handle them.

@matthewkmayer matthewkmayer added this to the 0.32.0 milestone Jan 23, 2018

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 25, 2018

Member

Rebased on current master to resolve conflicts, integration tests are ☑️

Member

srijs commented Jan 25, 2018

Rebased on current master to resolve conflicts, integration tests are ☑️

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 27, 2018

Member

This is top of my Rusoto to-do list. Planning on getting it reviewed and 🤞 merged this weekend.

Member

matthewkmayer commented Jan 27, 2018

This is top of my Rusoto to-do list. Planning on getting it reviewed and 🤞 merged this weekend.

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 27, 2018

Member

@matthewkmayer Can't like this enough! 😃

Member

srijs commented Jan 27, 2018

@matthewkmayer Can't like this enough! 😃

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 27, 2018

Member

Hitting a speed bump: #939 .

Member

matthewkmayer commented Jan 27, 2018

Hitting a speed bump: #939 .

@matthewkmayer

This comment has been minimized.

Show comment
Hide comment
@matthewkmayer

matthewkmayer Jan 28, 2018

Member

I don't see a reason not to merge this.

Longer version: this PR gets us async hyper ( 🎉 ) which we've wanted for a long time and integration tests pass. The only thing impeding us is my lack of knowledge and experience with futures and tokio. Due to other commitments and projects, I don't see that changing in the near future, pun not intended. Others more familiar with those libraries have already weighed in with reviews so I'm not overly concerned.

Also, @srijs is a maintainer so he can quickly issue fixes if issues crop up. 😁

I say we ship it, get the changes in for adding timeouts back in and ask people to give the new functionality a try before we release a new version. 👍

Member

matthewkmayer commented Jan 28, 2018

I don't see a reason not to merge this.

Longer version: this PR gets us async hyper ( 🎉 ) which we've wanted for a long time and integration tests pass. The only thing impeding us is my lack of knowledge and experience with futures and tokio. Due to other commitments and projects, I don't see that changing in the near future, pun not intended. Others more familiar with those libraries have already weighed in with reviews so I'm not overly concerned.

Also, @srijs is a maintainer so he can quickly issue fixes if issues crop up. 😁

I say we ship it, get the changes in for adding timeouts back in and ask people to give the new functionality a try before we release a new version. 👍

@srijs

This comment has been minimized.

Show comment
Hide comment
@srijs

srijs Jan 28, 2018

Member

That makes a lot of sense to me!

I would definitely expect a longer release cycle to ensure we leave enough time for bugs to be discovered and squashed. The hyper 0.11 approach, including releasing early alpha/beta versions could be a good example to follow.

It may also be good to see how quickly the tokio reform plays out in the coming weeks, to make an informed call whether we want to ship 0.32 with our own reactor, or hold out for the new tokio default reactor (and potentially futures 0.2 as well as hyper 0.12).

Member

srijs commented Jan 28, 2018

That makes a lot of sense to me!

I would definitely expect a longer release cycle to ensure we leave enough time for bugs to be discovered and squashed. The hyper 0.11 approach, including releasing early alpha/beta versions could be a good example to follow.

It may also be good to see how quickly the tokio reform plays out in the coming weeks, to make an informed call whether we want to ship 0.32 with our own reactor, or hold out for the new tokio default reactor (and potentially futures 0.2 as well as hyper 0.12).

@matthewkmayer matthewkmayer merged commit 5df5e93 into rusoto:master Jan 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment