feat: Implement Hyper specific HttpTransport #105
Conversation
| pub fn new() -> Self { | ||
| let connector = hyper_util::client::legacy::connect::HttpConnector::new(); | ||
| let timeout_connector = TimeoutConnector::new(connector); | ||
| let client = HyperClient::builder(TokioExecutor::new()).build(timeout_connector); |
There was a problem hiding this comment.
We are instantiating a new tokio executor here and in a couple of other places I'll point out. Think we need to add the option to provide the executor, or should we wait until it is specifically requested?
There was a problem hiding this comment.
I am not sure if would want to extend the API surface with more external library dependencies unless we have a driver for it. I think, because we have this whole transport behind a trait, worse case scenario you can copy/paste this transport and use your own executor. Where this would have been more important when the transport wasn't replaceable.
| .build(); | ||
|
|
||
| let timeout_connector = TimeoutConnector::new(connector); | ||
| let client = HyperClient::builder(TokioExecutor::new()).build(timeout_connector); |
| timeout_connector.set_read_timeout(self.read_timeout); | ||
| timeout_connector.set_write_timeout(self.write_timeout); | ||
|
|
||
| let client = HyperClient::builder(TokioExecutor::new()).build(timeout_connector); |
| /// | ||
| /// let transport = MyTransport::new(); | ||
| /// let client = ClientBuilder::for_url("https://example.com/events")? | ||
| /// let client = ClientBuilder::for_url("https://sse.dev/test") |
There was a problem hiding this comment.
Do we know who maintains sse.dev?
There was a problem hiding this comment.
No, I don't. The site shows a "Brought to you by https://www.stubber.com/", but that's about as much as I know.
If you want, we could change it over to the live scores feed we control: http://live-test-scores.herokuapp.com/scores
| /// | ||
| /// This timeout applies when reading data from the connection. | ||
| /// There is no read timeout by default. | ||
| pub fn read_timeout(mut self, timeout: Duration) -> Self { |
There was a problem hiding this comment.
This is still just an initial socket read timeout, correct? We don't yet have a keepalive style timeout?
There was a problem hiding this comment.
It does sound like maybe hyper supports tcp keepalive though.
There was a problem hiding this comment.
No, this should be a real read timeout since we are using the timeout connector. If you don't send something every X seconds, this will drop the connection if configured low enough.
keelerm84
force-pushed
the
mk/sdk-1726/hyper-transport
branch
from
d5775c7 to
3aed52f
Compare
No description provided.