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

feat(client): redesign the Connect trait #1428

Closed
wants to merge 1 commit into from
Closed

Conversation

seanmonstar
Copy link
Member

The original Connect trait had some limitations:

  • There was no way to provide more details to the connector about how to
    connect, other than the Uri.
  • There was no way for the connector to return any extra information
    about the connected transport.
  • The Error was forced to be an std::io::Error.
  • The transport and future had 'static requirements.

As hyper gains HTTP/2 support, some of these things needed to be
changed. We want to allow the user to configure whether they hope to
us ALPN to start an HTTP/2 connection, and the connector needs to be
able to return back to hyper if it did so.

The new Connect2 trait is meant to solve this.

  • The connect method now receives a Destination type, instead of a
    Uri. This allows us to include additional data about how to connect.
  • The Future returned from connect now must be a Connected, which
    wraps the transport, and includes possibly extra data about what
    happened when connecting.

The Connect trait is deprecated, with the hopes of Connect2 taking
it's place in the next breaking release. For backwards compatibility,
any type that implements Connect now will automaticall implement
Connect2, ignoring any of the extra data from Destination and
Connected.

A step towards #304.


I've tested this branch with hyper-tls, and ensured that without a change, it still works, and that in a new branch, the new trait can be adopted. Unfortunately, ALPN isn't exposed in native-tls, so that would be needed before hyper-tls could make use of this.

cc @sfackler @carllerche

}

/// Returns whether this connection must negotiate HTTP/2 via ALPN.
pub fn h2(&self) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect users to be able to configure a hyper Client, stating whether to allow both HTTP/1 and HTTP/2, or to require only HTTP/2. To allow both, ALPN needs to send both protocols. A connector would query the Destination to determine what protocols it should try for.

I wasn't actually sure what to name the methods. They are meant to represent the desire of the user, but some desires may be wants or preferences, versus musts. Maybe some combination in this list is better?

  • must_h2
  • can_http1
  • can_http2
  • prefer_h2

}

/// Convert into the underlyinng Transport.
pub fn into_transport(self) -> T {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was added when I was prototyping in hyper-tls, and noticed that to wrap another Connect2, I needed to take the transport out of the Connected. It's kind of verbose, however, and loses whatever settings the previous connector may have set. Would map<F, U>(self, f: F) where F: FnOnce(T) -> U perhaps be a better fit?


/// Set that the connected transport negotiated HTTP/2 as it's
/// next protocol.
pub fn h2(&mut self) -> &mut Connected<T> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect the Connected type have some methods to be able to set what protocol was actually negotiated. Should this be setters instead, set_h2() etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this also seems like a good way for connectors to report back if it's an HTTP/1.1 proxy, and so the absolute-form should be used for h1.

@tafia This may interest you, as you developed hyper-proxy. Maybe something like connected.set_proxy() or set_http1_proxy()...

@seanmonstar seanmonstar force-pushed the connect2 branch 3 times, most recently from 955004c to ac1a31f Compare January 27, 2018 00:48
@carllerche
Copy link

I don't have much context here. I'm not sure what exactly Connect instances are supposed to do for hyper. I don't know why passing a Uri is required.

As I'm sure you are aware, I have my stab at a connect trait here. It seems to pan out well as it is.

@seanmonstar
Copy link
Member Author

@carllerche The problem with the trait in tokio-connect is that it doesn't give enough information. hyper tries to provide a higher level API for users, but some of the details of how HTTP works depends on the connection layer. The tokio-connect trait doesn't allow passing any argument, so it cannot be given a host name or even a socket address.

The Connect trait in hyper is meant to be a little higher level, allowing users to swap out the connect/transport layer, while still getting an easier-to-use HTTP client. This PR tries to encapsulate what information hyper needs to give to connect layer, and what that can give back to hyper, in order to determine how exactly it can speak HTTP over it.

@seanmonstar
Copy link
Member Author

After thinking that a connector might return some info, like being an HTTP proxy, and another might wrap that connector to provide TLS, the proposed design of returning Connected<T> means that details from the underlying return value will likely be lost when the wrapper takes the transport out.

So it may be better to change the return type to (T, Connected) instead of Connected<T>, and allow wrapping connectors to just append details to the returned Connected metadata type.

The original `Connect` trait had some limitations:

- There was no way to provide more details to the connector about how to
  connect, other than the `Uri`.
- There was no way for the connector to return any extra information
  about the connected transport.
- The `Error` was forced to be an `std::io::Error`.
- The transport and future had `'static` requirements.

As hyper gains HTTP/2 support, some of these things needed to be
changed. We want to allow the user to configure whether they hope to
us ALPN to start an HTTP/2 connection, and the connector needs to be
able to return back to hyper if it did so.

The new `Connect2` trait is meant to solve this.

- The `connect` method now receives a `Destination` type, instead of a
  `Uri`. This allows us to include additional data about how to connect.
- The `Future` returned from `connect` now must be a tuple of the
  transport, and a `Connected` metadata value. The `Connected` includes
  possibly extra data about what happened when connecting.

The `Connect` trait is deprecated, with the hopes of `Connect2` taking
it's place in the next breaking release. For backwards compatibility,
any type that implements `Connect` now will automatically implement
`Connect2`, ignoring any of the extra data from `Destination` and
`Connected`.
@seanmonstar seanmonstar added this to the 0.12 milestone Feb 21, 2018
seanmonstar added a commit that referenced this pull request Mar 14, 2018
The original `Connect` trait had some limitations:

- There was no way to provide more details to the connector about how to
  connect, other than the `Uri`.
- There was no way for the connector to return any extra information
  about the connected transport.
- The `Error` was forced to be an `std::io::Error`.
- The transport and future had `'static` requirements.

As hyper gains HTTP/2 support, some of these things needed to be
changed. We want to allow the user to configure whether they hope to
us ALPN to start an HTTP/2 connection, and the connector needs to be
able to return back to hyper if it did so.

The new `Connect` trait is meant to solve this.

- The `connect` method now receives a `Destination` type, instead of a
  `Uri`. This allows us to include additional data about how to connect.
- The `Future` returned from `connect` now must be a tuple of the
  transport, and a `Connected` metadata value. The `Connected` includes
  possibly extra data about what happened when connecting.

BREAKING CHANGE: Custom connectors should now implement `Connect`
  directly, instead of `Service`.

  Calls to `connect` no longer take `Uri`s, but `Destination`. There
  are `scheme`, `host`, and `port` methods to query relevant
  information.

  The returned future must be a tuple of the transport and `Connected`.
  If no relevant extra information is needed, simply return
  `Connected::new()`.

Closes #1428
@seanmonstar
Copy link
Member Author

The ALPN stuff has been deferred for when HTTP2 upgrades are added, but the change still allows for that to be added in the future. A way for connectors to signal that the connection is to a proxy has stayed. This has been merged into 0.12.x here, without the deprecations and instead just changes to Connect directly.

@seanmonstar seanmonstar deleted the connect2 branch March 14, 2018 21:35
seanmonstar added a commit that referenced this pull request Mar 14, 2018
The original `Connect` trait had some limitations:

- There was no way to provide more details to the connector about how to
  connect, other than the `Uri`.
- There was no way for the connector to return any extra information
  about the connected transport.
- The `Error` was forced to be an `std::io::Error`.
- The transport and future had `'static` requirements.

As hyper gains HTTP/2 support, some of these things needed to be
changed. We want to allow the user to configure whether they hope to
us ALPN to start an HTTP/2 connection, and the connector needs to be
able to return back to hyper if it did so.

The new `Connect` trait is meant to solve this.

- The `connect` method now receives a `Destination` type, instead of a
  `Uri`. This allows us to include additional data about how to connect.
- The `Future` returned from `connect` now must be a tuple of the
  transport, and a `Connected` metadata value. The `Connected` includes
  possibly extra data about what happened when connecting.

BREAKING CHANGE: Custom connectors should now implement `Connect`
  directly, instead of `Service`.

  Calls to `connect` no longer take `Uri`s, but `Destination`. There
  are `scheme`, `host`, and `port` methods to query relevant
  information.

  The returned future must be a tuple of the transport and `Connected`.
  If no relevant extra information is needed, simply return
  `Connected::new()`.

Closes #1428
seanmonstar added a commit that referenced this pull request Mar 19, 2018
The original `Connect` trait had some limitations:

- There was no way to provide more details to the connector about how to
  connect, other than the `Uri`.
- There was no way for the connector to return any extra information
  about the connected transport.
- The `Error` was forced to be an `std::io::Error`.
- The transport and future had `'static` requirements.

As hyper gains HTTP/2 support, some of these things needed to be
changed. We want to allow the user to configure whether they hope to
us ALPN to start an HTTP/2 connection, and the connector needs to be
able to return back to hyper if it did so.

The new `Connect` trait is meant to solve this.

- The `connect` method now receives a `Destination` type, instead of a
  `Uri`. This allows us to include additional data about how to connect.
- The `Future` returned from `connect` now must be a tuple of the
  transport, and a `Connected` metadata value. The `Connected` includes
  possibly extra data about what happened when connecting.

BREAKING CHANGE: Custom connectors should now implement `Connect`
  directly, instead of `Service`.

  Calls to `connect` no longer take `Uri`s, but `Destination`. There
  are `scheme`, `host`, and `port` methods to query relevant
  information.

  The returned future must be a tuple of the transport and `Connected`.
  If no relevant extra information is needed, simply return
  `Connected::new()`.

Closes #1428
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants