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

Port middleware connectors to the orchestrator #2970

Merged
merged 7 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,27 @@ message = "Required members with @contextParam are now treated as client-side re
references = ["smithy-rs#2964"]
meta = { "breaking" = false, "tada" = false, "bug" = false, target = "client" }
author = "rcoh"

[[smithy-rs]]
message = "`aws_smithy_client::hyper_ext::Adapter` was moved/renamed to `aws_smithy_runtime::client::connectors::hyper_connector::HyperConnector`."
references = ["smithy-rs#2970"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"

[[smithy-rs]]
message = "Test connectors moved into `aws_smithy_runtime::client::connectors::test_util` behind the `test-util` feature."
references = ["smithy-rs#2970"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"

[[smithy-rs]]
message = "DVR's RecordingConnection and ReplayingConnection were renamed to RecordingConnector and ReplayingConnector respectively."
references = ["smithy-rs#2970"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"

[[smithy-rs]]
message = "TestConnection was renamed to EventConnector."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree the name is a little more descriptive, we should probably introduce some sort of table-of-contents of test connections

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Added in 17a518c

references = ["smithy-rs#2970"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a doc explaining the difference between Connection and Connector?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked about in 17a518c

6 changes: 2 additions & 4 deletions rust-runtime/aws-smithy-client/src/erase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,8 @@ impl DynConnector {
pub fn call_lite(
&mut self,
req: http::Request<SdkBody>,
) -> BoxFuture<http::Response<SdkBody>, Box<dyn std::error::Error + Send + Sync + 'static>>
{
let future = Service::call(self, req);
Box::pin(async move { future.await.map_err(|err| Box::new(err) as _) })
) -> BoxFuture<http::Response<SdkBody>, ConnectorError> {
Service::call(self, req)
}
}

Expand Down
15 changes: 12 additions & 3 deletions rust-runtime/aws-smithy-runtime-api/src/client/connectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,19 @@
* SPDX-License-Identifier: Apache-2.0
*/

use crate::client::orchestrator::{BoxFuture, HttpRequest, HttpResponse};
use crate::client::orchestrator::{HttpRequest, HttpResponse};
use aws_smithy_async::future::now_or_later::NowOrLater;
use aws_smithy_http::result::ConnectorError;
use std::fmt;
use std::future::Future as StdFuture;
use std::pin::Pin;
use std::sync::Arc;

/// Boxed future used by [`HttpConnectorFuture`].
pub type BoxFuture = Pin<Box<dyn StdFuture<Output = Result<HttpResponse, ConnectorError>> + Send>>;
/// Future for [`HttpConnector::call`].
pub type HttpConnectorFuture = NowOrLater<Result<HttpResponse, ConnectorError>, BoxFuture>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we introduce a new-type future instead? That seems like it would afford us more API evolution in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced in 8eb9923


/// Trait with a `call` function that asynchronously converts a request into a response.
///
/// Ordinarily, a connector would use an underlying HTTP library such as [hyper](https://crates.io/crates/hyper),
Expand All @@ -16,7 +25,7 @@ use std::sync::Arc;
/// for testing.
pub trait HttpConnector: Send + Sync + fmt::Debug {
/// Asynchronously converts a request into a response.
fn call(&self, request: HttpRequest) -> BoxFuture<HttpResponse>;
fn call(&self, request: HttpRequest) -> HttpConnectorFuture;
}

/// A shared [`HttpConnector`] implementation.
Expand All @@ -31,7 +40,7 @@ impl SharedHttpConnector {
}

impl HttpConnector for SharedHttpConnector {
fn call(&self, request: HttpRequest) -> BoxFuture<HttpResponse> {
fn call(&self, request: HttpRequest) -> HttpConnectorFuture {
(*self.0).call(request)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -511,11 +511,11 @@ impl RuntimeComponentsBuilder {
#[cfg(feature = "test-util")]
pub fn for_tests() -> Self {
use crate::client::auth::AuthSchemeOptionResolver;
use crate::client::connectors::HttpConnector;
use crate::client::connectors::{HttpConnector, HttpConnectorFuture};
use crate::client::endpoint::{EndpointResolver, EndpointResolverParams};
use crate::client::identity::Identity;
use crate::client::identity::IdentityResolver;
use crate::client::orchestrator::Future;
use crate::client::orchestrator::{Future, HttpRequest};
use crate::client::retries::RetryStrategy;
use aws_smithy_async::rt::sleep::AsyncSleep;
use aws_smithy_async::time::TimeSource;
Expand All @@ -537,11 +537,7 @@ impl RuntimeComponentsBuilder {
#[derive(Debug)]
struct FakeConnector;
impl HttpConnector for FakeConnector {
fn call(
&self,
_: crate::client::orchestrator::HttpRequest,
) -> crate::client::orchestrator::BoxFuture<crate::client::orchestrator::HttpResponse>
{
fn call(&self, _: HttpRequest) -> HttpConnectorFuture {
unreachable!("fake connector must be overridden for this test")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,8 @@ impl RuntimePlugins {
#[cfg(test)]
mod tests {
use super::{RuntimePlugin, RuntimePlugins};
use crate::client::connectors::{HttpConnector, SharedHttpConnector};
use crate::client::orchestrator::{BoxFuture, HttpRequest, HttpResponse};
use crate::client::connectors::{HttpConnector, HttpConnectorFuture, SharedHttpConnector};
use crate::client::orchestrator::HttpRequest;
use crate::client::runtime_components::RuntimeComponentsBuilder;
use crate::client::runtime_plugin::Order;
use aws_smithy_http::body::SdkBody;
Expand Down Expand Up @@ -342,29 +342,29 @@ mod tests {
#[derive(Debug)]
struct CN1;
impl HttpConnector for CN1 {
fn call(&self, _: HttpRequest) -> BoxFuture<HttpResponse> {
Box::pin(async {
fn call(&self, _: HttpRequest) -> HttpConnectorFuture {
HttpConnectorFuture::new(Box::pin(async {
Ok(http::Response::builder()
.status(200)
.header("rp1", "1")
.body(SdkBody::empty())
.unwrap())
})
}))
}
}

// CN2, the outer connector, calls the inner connector and adds the `rp2` header to the response
#[derive(Debug)]
struct CN2(SharedHttpConnector);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cn2 sounds like a protocol but I guess this is a test connector? maybe should be renamed Connector2 or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in 8eb9923

impl HttpConnector for CN2 {
fn call(&self, request: HttpRequest) -> BoxFuture<HttpResponse> {
fn call(&self, request: HttpRequest) -> HttpConnectorFuture {
let inner = self.0.clone();
Box::pin(async move {
HttpConnectorFuture::new(Box::pin(async move {
let mut resp = inner.call(request).await.unwrap();
resp.headers_mut()
.append("rp2", HeaderValue::from_static("1"));
Ok(resp)
})
}))
}
}

Expand Down
10 changes: 9 additions & 1 deletion rust-runtime/aws-smithy-runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ repository = "https://github.com/awslabs/smithy-rs"
[features]
client = ["aws-smithy-runtime-api/client"]
http-auth = ["aws-smithy-runtime-api/http-auth"]
test-util = ["aws-smithy-runtime-api/test-util", "dep:aws-smithy-protocol-test", "dep:tracing-subscriber"]
test-util = ["aws-smithy-runtime-api/test-util", "dep:aws-smithy-protocol-test", "dep:tracing-subscriber", "dep:serde", "dep:serde_json"]
connector-hyper = ["dep:hyper", "hyper?/client", "hyper?/http2", "hyper?/http1", "hyper?/tcp"]
tls-rustls = ["dep:hyper-rustls", "dep:rustls", "connector-hyper"]

[dependencies]
aws-smithy-async = { path = "../aws-smithy-async" }
Expand All @@ -25,9 +27,14 @@ bytes = "1"
fastrand = "2.0.0"
http = "0.2.8"
http-body = "0.4.5"
hyper = { version = "0.14.26", default-features = false, optional = true }
hyper-rustls = { version = "0.24", features = ["rustls-native-certs", "http2"], optional = true }
once_cell = "1.18.0"
pin-project-lite = "0.2.7"
pin-utils = "0.1.0"
rustls = { version = "0.21.1", optional = true }
serde = { version = "1", features = ["derive"], optional = true }
serde_json = { version = "1", optional = true }
tokio = { version = "1.25", features = [] }
tracing = "0.1.37"
tracing-subscriber = { version = "0.3.16", optional = true, features = ["fmt", "json"] }
Expand All @@ -37,6 +44,7 @@ approx = "0.5.1"
aws-smithy-async = { path = "../aws-smithy-async", features = ["rt-tokio", "test-util"] }
aws-smithy-runtime-api = { path = "../aws-smithy-runtime-api", features = ["test-util"] }
aws-smithy-types = { path = "../aws-smithy-types", features = ["test-util"] }
hyper-tls = { version = "0.5.0" }
tokio = { version = "1.25", features = ["macros", "rt", "test-util"] }
tracing-subscriber = { version = "0.3.16", features = ["env-filter"] }
tracing-test = "0.2.1"
Expand Down
12 changes: 8 additions & 4 deletions rust-runtime/aws-smithy-runtime/src/client/connectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@ pub mod connection_poisoning;
#[cfg(feature = "test-util")]
pub mod test_util;

/// Default HTTP and TLS connectors that use hyper and rustls.
#[cfg(feature = "connector-hyper")]
pub mod hyper_connector;

// TODO(enableNewSmithyRuntimeCleanup): Delete this module
/// Unstable API for interfacing the old middleware connectors with the newer orchestrator connectors.
///
/// Important: This module and its contents will be removed in the next release.
pub mod adapter {
use aws_smithy_client::erase::DynConnector;
use aws_smithy_runtime_api::client::connectors::HttpConnector;
use aws_smithy_runtime_api::client::orchestrator::{BoxFuture, HttpRequest, HttpResponse};
use aws_smithy_runtime_api::client::connectors::{HttpConnector, HttpConnectorFuture};
use aws_smithy_runtime_api::client::orchestrator::HttpRequest;
use std::sync::{Arc, Mutex};

/// Adapts a [`DynConnector`] to the [`HttpConnector`] trait.
Expand All @@ -40,9 +44,9 @@ pub mod adapter {
}

impl HttpConnector for DynConnectorAdapter {
fn call(&self, request: HttpRequest) -> BoxFuture<HttpResponse> {
fn call(&self, request: HttpRequest) -> HttpConnectorFuture {
let future = self.dyn_connector.lock().unwrap().call_lite(request);
future
HttpConnectorFuture::new(future)
}
}
}
Loading