-
Notifications
You must be signed in to change notification settings - Fork 195
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
Introduce a Standard
enum for connections
#237
Changes from 11 commits
8e1be0d
2561f77
37e9c69
d2f2e2d
2690cce
2a01948
41a44fb
4ac1288
4bafbd3
1a6eec0
2d37ba4
3dbb487
b01c740
abea9e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0. | ||
*/ | ||
|
||
use crate::BoxError; | ||
use http::Request; | ||
use hyper::client::{HttpConnector, ResponseFuture}; | ||
use hyper::Response; | ||
use hyper_tls::HttpsConnector; | ||
use smithy_http::body::SdkBody; | ||
use std::future::{Future, Ready}; | ||
use std::pin::Pin; | ||
use std::task::{Context, Poll}; | ||
use tower::Service; | ||
|
||
#[derive(Clone)] | ||
pub struct Standard(Connector); | ||
|
||
impl Standard { | ||
/// An https connection | ||
pub fn https() -> Self { | ||
let https = HttpsConnector::new(); | ||
Self(Connector::Https(hyper::Client::builder().build::<_, SdkBody>(https))) | ||
} | ||
|
||
/// A connection based on the provided `Box<dyn HttpService>` | ||
/// | ||
/// Generally, `https()` should be used instead. This constructor is intended to support | ||
/// using things like [`TestConnection`](crate::test_connection::TestConnection) or alternative | ||
/// http implementations. | ||
pub fn new(connector: Box<dyn HttpService>) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is generally only used for tests, can we make it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's used for external users to test their usages of the connection (eg. for integration tests I would swap in a |
||
Self(Connector::Dyn(connector)) | ||
} | ||
} | ||
|
||
/// An Http connection type for most use cases | ||
/// | ||
/// This supports three options: | ||
/// 1. HTTPS | ||
/// 2. A `TestConnection` | ||
/// 3. Any implementation of the `HttpService` trait | ||
/// | ||
/// This is designed to be used with [`aws_hyper::Client`](crate::Client) as a connector. | ||
enum Connector { | ||
/// An Https Connection | ||
/// | ||
/// This is the correct connection for use cases talking to real AWS services. | ||
Https(hyper::Client<HttpsConnector<HttpConnector>, SdkBody>), | ||
|
||
/// A generic escape hatch | ||
/// | ||
/// This enables using any implementation of the HttpService trait. This allows using a totally | ||
/// separate HTTP stack or your own custom `TestConnection`. | ||
Dyn(Box<dyn HttpService>), | ||
} | ||
|
||
impl Clone for Connector { | ||
fn clone(&self) -> Self { | ||
match self { | ||
Connector::Https(client) => Connector::Https(client.clone()), | ||
Connector::Dyn(box_conn) => Connector::Dyn(box_conn.clone()), | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i assume you implemented this because it couldn't be derived? i thought it would be able to... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh good catch. I think this was from before I added |
||
|
||
impl Clone for Box<dyn HttpService> { | ||
fn clone(&self) -> Self { | ||
self.clone_box() | ||
} | ||
} | ||
|
||
pub trait HttpService: Send { | ||
/// Return whether this service is ready to accept a request | ||
/// | ||
/// See [`Service::poll_ready`](tower::Service::poll_ready) | ||
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), BoxError>>; | ||
|
||
/// Call this service and return a response | ||
/// | ||
/// See [`Service::call`](tower::Service::call) | ||
fn call( | ||
&mut self, | ||
req: http::Request<SdkBody>, | ||
) -> Pin<Box<dyn Future<Output = Result<http::Response<hyper::Body>, BoxError>> + Send>>; | ||
|
||
/// Return a Boxed-clone of this service | ||
/// | ||
/// `aws_hyper::Client` will clone the inner service for each request so this should be a cheap | ||
/// clone operation. | ||
fn clone_box(&self) -> Box<dyn HttpService>; | ||
} | ||
|
||
/// Reverse implementation: If you have a correctly shaped tower service, it _is_ an `HttpService` | ||
/// | ||
/// This is to facilitate ease of use for people using `Standard::Dyn` | ||
impl<S> HttpService for S | ||
where | ||
S: Service<http::Request<SdkBody>, Response = http::Response<hyper::Body>> | ||
+ Send | ||
+ Clone | ||
+ 'static, | ||
S::Error: Into<BoxError> + Send + Sync + 'static, | ||
S::Future: Send + 'static, | ||
{ | ||
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), BoxError>> { | ||
Service::poll_ready(self, cx).map_err(|err| err.into()) | ||
} | ||
|
||
fn call( | ||
&mut self, | ||
req: Request<SdkBody>, | ||
) -> Pin<Box<dyn Future<Output = Result<Response<hyper::Body>, BoxError>> + Send>> { | ||
let fut = Service::call(self, req); | ||
let fut = async move { fut.await.map_err(|err| err.into()) }; | ||
Box::pin(fut) | ||
} | ||
|
||
fn clone_box(&self) -> Box<dyn HttpService> { | ||
Box::new(self.clone()) | ||
} | ||
} | ||
|
||
impl tower::Service<http::Request<SdkBody>> for Standard { | ||
type Response = http::Response<hyper::Body>; | ||
type Error = BoxError; | ||
type Future = StandardFuture; | ||
|
||
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
match &mut self.0 { | ||
Connector::Https(https) => Service::poll_ready(https, cx).map_err(|err| err.into()), | ||
Connector::Dyn(conn) => conn.poll_ready(cx), | ||
} | ||
} | ||
|
||
fn call(&mut self, req: http::Request<SdkBody>) -> Self::Future { | ||
match &mut self.0 { | ||
Connector::Https(https) => StandardFuture::Https(Service::call(https, req)), | ||
Connector::Dyn(conn) => StandardFuture::Dyn(conn.call(req)), | ||
} | ||
} | ||
} | ||
|
||
/// Future returned by `Standard` when used as a tower::Service | ||
#[pin_project::pin_project(project = FutProj)] | ||
pub enum StandardFuture { | ||
Https(#[pin] ResponseFuture), | ||
TestConn(#[pin] Ready<Result<http::Response<hyper::Body>, BoxError>>), | ||
Dyn(#[pin] Pin<Box<dyn Future<Output = Result<http::Response<hyper::Body>, BoxError>> + Send>>), | ||
} | ||
|
||
impl Future for StandardFuture { | ||
type Output = Result<http::Response<hyper::Body>, BoxError>; | ||
|
||
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
match self.project() { | ||
FutProj::TestConn(ready_fut) => ready_fut.poll(cx), | ||
FutProj::Https(fut) => fut.poll(cx).map_err(|err| err.into()), | ||
FutProj::Dyn(dyn_fut) => dyn_fut.poll(cx), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
StandardConnection
or something like that? the nameStandard
is pretty vague when taken out of the module context (re-exports and the like).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally agree, but the other side of the house requested that I follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html#avoid-redundant-prefixes-[rfc-356] 🤷🏻
I guess we should pick and stick to a convention, but I don't have strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could rename the module as
connection
? orconnector
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ehh, i think
conn
is fine. i wasn't aware of that style guide suggestion