Intial Client Configuration #79
Conversation
|
It looks like this will also fix #79 |
src/client.rs
Outdated
| @@ -38,14 +35,20 @@ pub struct Body<B: IntoBuf> { | |||
| inner: proto::StreamRef<B::Buf, Peer>, | |||
| } | |||
|
|
|||
| /// Configure a Client from the defaults. | |||
| #[derive(Debug, Default)] | |||
| pub struct Config { | |||
There was a problem hiding this comment.
I would consider renaming this to Builder as this looks more like a builder pattern.
src/client.rs
Outdated
| @@ -60,41 +63,22 @@ impl<T, B> Client<T, B> | |||
| /// | |||
| /// It's important to note that this does not **flush** the outbound | |||
| /// settings to the wire. | |||
| pub fn handshake2(io: T) -> Handshake<T, B> { | |||
| pub fn handshake2(io: T, config: Config) -> Handshake<T, B> { | |||
There was a problem hiding this comment.
I think I would either call it Config and pass it as an arg here (and remove handshake2 on Config) or call it Builder and remove pub from this fn.
I think that I prefer the builder approach personally.
src/client.rs
Outdated
| let settings = self.config.settings.clone(); | ||
|
|
||
| // Send initial settings frame | ||
| match codec.start_send(settings.clone().into()) { |
There was a problem hiding this comment.
Since you are here, could you switch this to use Codec::buffer instead.
src/client.rs
Outdated
| self | ||
| } | ||
|
|
||
| pub fn handshake<T>(&self, io: T) -> Handshake<T, Bytes> |
There was a problem hiding this comment.
I wonder if this fn should be removed and handshake2 renamed to handshake. Then Client::handshake could be the "simple" option.
src/proto/connection.rs
Outdated
| @@ -56,13 +56,14 @@ impl<T, P, B> Connection<T, P, B> | |||
| P: Peer, | |||
| B: IntoBuf, | |||
| { | |||
| pub fn new(codec: Codec<T, Prioritized<B::Buf>>) -> Connection<T, P, B> { | |||
| pub fn new(codec: Codec<T, Prioritized<B::Buf>>, settings: frame::Settings) -> Connection<T, P, B> { | |||
There was a problem hiding this comment.
This could probably be passed in by ref and it would avoid the clone / move from above.
src/proto/streams/recv.rs
Outdated
| // The spec gives us the option of returning either a connection error | ||
| // or stream error when a stream overflows its window (but not the | ||
| // connection). So, we opt to send a stream error. | ||
| if stream.recv_flow.window_size() < sz { |
There was a problem hiding this comment.
If you use a stream level error here, then sz has to be applied against the connection level flow control.
A receiver that receives a flow-controlled frame MUST always account for its contribution against the connection flow-control window, unless the receiver treats this as a connection error (Section 5.4.1). This is necessary even if the frame is in error. The sender counts the frame toward the flow-control window, but if the receiver does not, the flow-control window at the sender and receiver can become different.
This relates to #32 and generally speaking there probably are existing bugs related to this in the repo.
src/proto/streams/recv.rs
Outdated
| stream.recv_task = Some(task::current()); | ||
| Ok(Async::NotReady) | ||
| }, | ||
| Err(proto::Error::Proto(Reason::NoError)) => { |
There was a problem hiding this comment.
Could you explain this logic? I'm not sure I follow when ensure_recv_open would return NoError and why this is mapped as an end of stream.
There was a problem hiding this comment.
This would return NoError when either half closed, or closed with no error. This is treating the NoError variant similar to how IO treats the WouldBlock variant: for the purposes of this function, it's not an error, but means the stream is closed.
Alternatively, I can change ensure_recv_open to return a Poll<(), Error>, and convert NoError into an Ok(NotReady) in that function, and just pass it along here.
|
Comments addressed, replied about |
|
Ack, sorry... looks like I merged your rustfmt PR first :'( |
|
Updated with new formatting again. |
|
Adjusted |
Only started with a single setting,
initial_window_size, instead of offer a bunch that aren't actually hooked up. As we want more (and we do), we will have to be sure that the setting actually does what it says.While I was working on this, and coming up with a test case to make sure it worked, I ended up fixing some other things too... Oh well, they're broken into logical commits.
cc #40 (server config still missing)