Try h2 on h1 failure #1503
Try h2 on h1 failure #1503
Conversation
|
So, I'm a bit stuck at the moment, my current strategy is to wrap the I'm fairly new to rust so I feel like I'm a bit out of my depth here, and would really appreciate any help. |
|
So, that specific error is probably a red herring, but I can explain the problem:
I don't think you'll need an |
|
Hey Sean, |
|
@seanmonstar So, I've now gone down the deconstruction path, and I saw two options:
By the looks of it, by wrapping in the enum I'd have to implement the Dispatch trait on it which seems like a lot of boilerplate and hence probably not the right way to go. After giving the first option a try I find myself again a bit stuck and would love a tip if you have a sec, I'm getting the following compiler message which is a bit ironic since I dont really care what the type is on client since it never returns |
|
You shouldn't need to to much with the impl<generics> Dispatch<io, etc, Server<S>> {
pub fn hello() {}
}Though, it probably does mean the the |
|
Hokay, stuck again. Current issue is below, my guess is that since we are parameterizing for a specific PS I wrote the RewindedAddrStream because I didn't see a way to rewind io given the type bounds AsyncRead + AsyncWrite. Is that true? |
|
Thanks for sticking with this! I maybe should have labelled this hard instead of medium, it seems it's more involved than I originally thought XD Yes, we'd need some sort of wrapper for the generic |
|
Haha, thanks Sean, yeah part of this is that I've got about 1 month of rust experience. Meanwhile in a separate term: I'm thinking ive still dropped some bytes somewhere, so that will be my next investigation, I'm thinking I'll tcpdump the traffic and compare with what is available in the rewound io object that I pass to |
|
Working, see pr description. |
src/server/mod.rs
Outdated
| @@ -82,6 +83,12 @@ pub struct Builder<I> { | |||
| protocol: Http_, | |||
| } | |||
|
|
|||
| /// AsyncRewind is a rewindable AsyncRead+AsyncWrite trait | |||
| pub trait AsyncRewind: AsyncRead + AsyncWrite { | |||
There was a problem hiding this comment.
I think we can probably build the rewinding in internal, and not require that ever transport type a user might want to use have to implement this function. Basically, a private struct Rewound<I>. Here's a way I'd do it:
- In the h1 case, nothing should be needed there. If there is a parse error, we can just use
deconstruct/into_innerto get the original transport and theread_bufback. - If we've done number 1, and want to start the h2 handshake, then a
Rewoundwould be constructed with those 2 pieces, and thenRewound<I>would be theAsyncRead + AsyncWritetype passed to the h2 handshake/connection.
That'd make the server::Connection type look kind of like this internally:
pub struct Connection<I, S> {
inner: Option<
Either<
h1::Dispatcher<..., I>,
h2::Server<Rewound<I>, ...>,
>
>
}There was a problem hiding this comment.
I really like this idea, part of the PR cleanup I was planning was to try to internalize (to the api) all of my changes and I think this is a perfect way to do it! The only other thing I was thinking about was peeking at the first few bytes in Connection and checking against h2::PREFACE before even going down the path of either h1 or h2. The reason for taking this approach would be that you would have a clean separation between h1 and h2. The other benefit is that we dont even have to back stuff out of h1 if we check for the preface.
Thoughts?
PS I really appreciate all the mentoring on this one, I feel like I've learned so much about rust ecosystem just on this one pr. 😄
There was a problem hiding this comment.
That could work... I'm not sure I've thought through all the ramifications of that. We couldn't actual require MSG_PEEK, since that specific to TCP, and the IO type could be anything. Hopefully, handling of errors is still similar, so that h1 parse errors still send an appropriate HTTP/1 response.
Though, this would solve an issue I hadn't commented about yet, which is that the h2 fallover should only be applicable on the very first request. If the stream has already spoken h1, a later parse error shouldn't try to upgrade to h2.
src/proto/h1/conn.rs
Outdated
| @@ -545,6 +552,9 @@ where I: AsyncRead + AsyncWrite, | |||
| // - Client: there is nothing we can do | |||
| // - Server: if Response hasn't been written yet, we can send a 4xx response | |||
| fn on_parse_error(&mut self, err: ::Error) -> ::Result<()> { | |||
| if self.shutdown_on_error == false { | |||
There was a problem hiding this comment.
This could probably be inside T::on_error instead. Hm, it could perhaps be useful if the on_error method were adjusted to allow accessing the current &read_buf. Then, ServerTransaction::on_error could check to see if it at least starts with the HTTP/2 magic prefix before deciding to bubble the error back up (and let server::Connection try h2), or to just return the HTTP/1 bad response.
estk
force-pushed
the
try_h2_on_h1_failure
branch
2 times, most recently
from
4374a2d
to
256ce6d
Compare
|
You were totally right, it looks like it might be a bit of a nightmare to add the checking to the |
estk
force-pushed
the
try_h2_on_h1_failure
branch
4 times, most recently
from
9ae9c9a
to
155507b
Compare
|
@seanmonstar I'm a bit stuck again, it looks to me like things should be working, any idea why the ci seems to only pass on nightly? |
|
@estk looking at the logs right now, it seems to be complaining about illegal syntax. It looks like a merge conflict was saved, as there's a line like |
|
Lol, that’s embarrassing, i guess ill blame that on not having coffee yet. The build that i was referring to was the one before. Im mobile now but ill fix it up this evening |
There was a problem hiding this comment.
@seanmonstar, the ci issue I was referring to exists on the current build: https://travis-ci.org/hyperium/hyper/builds/375446742. I'm not really sure what to make of it since it was only on nightly with a specific set of options.
src/server/conn.rs
Outdated
| @@ -513,3 +553,165 @@ where | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // TODO: different file? idk where to put this. | |||
| mod rewindable { | |||
There was a problem hiding this comment.
@seanmonstar, it looks like hyper as a project prefers to keep new files to a minimum, is this an appropriate place to put this?
There was a problem hiding this comment.
There isn't an enforced preference, really... keeping things in less files came from two reasons: a) it can get annoying to have to import a bunch of the same stuff over and over, and b) sometimes some types make use of private fields of others (though this is less important now that there exists pub(scope)s.
If you think it's clearer in a separate file, that's perfectly fine :)
| @@ -39,6 +40,24 @@ fn tcp_bind(addr: &SocketAddr, handle: &Handle) -> ::tokio::io::Result<TcpListen | |||
| TcpListener::from_std(std_listener, handle) | |||
| } | |||
|
|
|||
| #[test] | |||
| fn try_h2() { | |||
There was a problem hiding this comment.
@seanmonstar this is a significant deviation from how other tests are performed, but I'm not really sure how else I would go about it. Any guidance here?
There was a problem hiding this comment.
It's fine! I've really wanted to take some time to clean up the tests, they've become a mess. They started with just a server helper, but eventually more specific details needed to be tested, and I never took the time to group up boilerplate.
src/proto/h1/dispatch.rs
Outdated
| T: Http1Transaction, | ||
| Bs: Payload, | ||
| { | ||
| pub fn deconstruct(self) -> (I, Bytes, S) { |
There was a problem hiding this comment.
I think this can just be Dispatcher::into_inner, right?
src/server/conn.rs
Outdated
| } | ||
|
|
||
| /// Deconstructed parts of a `Connection`. | ||
| /// | ||
| /// This allows taking apart a `Connection` at a later time, in order to | ||
| /// reclaim the IO object, and additional related pieces. | ||
| #[derive(Debug)] | ||
| pub struct Parts<T, S> { | ||
| pub struct Parts<T, S> | ||
| where S: Service { |
There was a problem hiding this comment.
Is this new bounds required?
There was a problem hiding this comment.
It seems not, ive removed it.
So I know for the future, Is it a rust thing to not include bounds that will be satisfied but are not required? I would think that since this is an intermediate data structure we would want to be as specific as possible about what it contains for the end user.
src/server/conn.rs
Outdated
| Either::A(ref mut h1) => { | ||
| try_ready!(h1.poll_without_shutdown()); | ||
| Ok(().into()) | ||
| }, | ||
| Either::B(ref mut h2) => h2.poll(), | ||
| } | ||
| } | ||
|
|
||
| fn try_h2(&mut self) -> Poll<(), ::Error> { | ||
| debug!("Trying to upgrade to h2"); |
There was a problem hiding this comment.
May want to demote this to a trace!, and remove the other two debug! logs in this function.
src/server/conn.rs
Outdated
| #[derive(Debug)] | ||
| pub struct Rewindable<T> | ||
| where | ||
| T: AsyncRead + AsyncWrite, |
There was a problem hiding this comment.
Ah, I see. If these bounds are removed, then they won't be needed in several other places as well. Usually, prefer bounds only on impls that need them (sometimes it's unavoidable in a struct definition, if you need to refer to an associated type).
There was a problem hiding this comment.
This is a great rust nugget of knowledge, thanks.
src/server/conn.rs
Outdated
| } | ||
| } | ||
| pub fn prepend(&mut self, bs: Bytes) { | ||
| self.pre.get_or_insert(bs); |
There was a problem hiding this comment.
Prefer self.pre = Some(bs) instead, since get_or_insert does some matching on the original field, but we aren't using it.
You could put a debug_assert!(self.pre.is_none()) before setting it, for correctness while tests are run.
src/server/conn.rs
Outdated
|
|
||
| #[inline] | ||
| fn read_buf<B: BufMut>(&mut self, buf: &mut B) -> Poll<usize, io::Error> { | ||
| if let Some(ref mut bs) = self.pre { |
There was a problem hiding this comment.
Perhaps here and in read, the pre should be changed to None once all the bytes have been read. That will mean then skipping the rest of this if block on latter reads, and allow the Bytes to be freed since it's no longer needed.
There was a problem hiding this comment.
Absolutely, actually I'm not super confident on what is in here, but getting close to something that seems right.
src/server/rewindable.rs
Outdated
| // then we could have two separate code paths where we dont need to stage changes like | ||
| // here which adds complexity. | ||
| #[inline] | ||
| fn read_buf<B: BufMut>(&mut self, buf: &mut B) -> Poll<usize, io::Error> { |
There was a problem hiding this comment.
@seanmonstar do you know if there are any guarantees around how many bytes should be written onto buf? If there are none we could drastically simplify this code.
src/server/rewindable.rs
Outdated
|
|
||
| let mut o2_cursor = Cursor::new(o2); | ||
| stream.read_buf(&mut o2_cursor).unwrap(); | ||
| stream.read_buf(&mut o2_cursor).unwrap(); |
There was a problem hiding this comment.
@seanmonstar This the crux of what im concerned about, is it okay that we're not reading out all bytes from a perf/correctness point of view?
There was a problem hiding this comment.
I don't actually understand the question, or what this test is testing :D
Mind adding some comments about what each part of the test is triggering or why?
There was a problem hiding this comment.
Haha sorry, was a bit of a non-question. I think ive answered it myself, I was wondering if there is an undocumented dependency of the read_buf method to read all the bytes possible into the buf in a single call.
How I'm doing it here is one of the following per call:
- Is there a prefix? Read as many bytes in the prefix as possible out, If there are some left, put those back in the prefix field.
- If there is not a prefix, read direct from the
inner
In practice this means the buf passed in will not always be filled even though there may be bytes ready to read. I can imagine a situation where there is code relying on a small buffer being completely filled which could break.
There was a problem hiding this comment.
Oh, I see what you mean. Yea, this behavior is correct. You don't want to copy the buffered bytes, and try to a syscall in the same read. Because: what's the behavior if there is an error from the syscall? Return the error? But then the buffered bytes... what about those? It's messy.
Plus, anyone relying on a read to completely fill a buffer is definitely using IO incorrectly. You never know exactly how much data might be in the kernels buffers, and you don't know if more arrived just as it partially filled your buffer, so you just have to keep reading until your buffer is full, or your get WouldBlock.
src/server/rewindable.rs
Outdated
| if let Some(bs) = self.pre.take() { | ||
| // If there are no remaining bytes, let the bytes get dropped. | ||
| if bs.len() > 0 { | ||
| let mut reader = bs.into_buf().reader(); |
There was a problem hiding this comment.
I think there may be a more idiomatic way to copy between a Buf and a BufMut. I think this may work:
let cnt = buf.remaining_mut();
let mut bs = bs.into_buf().take(cnt);
buf.put(&mut bs);
// bs.into_inner() stuffIt's unfortunate the dance needed for BytesMut. It's already been decided that it was mistake that it doesn't implement Buf itself, but it can't without a breaking change. It will in bytes 0.5.
There was a problem hiding this comment.
Excellent info as always.
|
@seanmonstar one thing that i cant seem to figure out is why the ci tests fail (but not for every build option) while locally they seem to be fine. Any ideas? |
|
There seems to be some sort of race condition in h2 that sometimes prevents the client connection from completely closing up, resulting in the test server and client just staring at each other until Travis gives up. 😭 |
estk
force-pushed
the
try_h2_on_h1_failure
branch
2 times, most recently
from
9e35904
to
4a006ad
Compare
estk
force-pushed
the
try_h2_on_h1_failure
branch
2 times, most recently
from
d61e147
to
7d58680
Compare
|
@seanmonstar whew, it looks like we're approaching the finish line on this one. Any further comments or tests you'd like me to add? |
There was a problem hiding this comment.
You're a champ! Thanks for the continued effort on this! The comments I left are pretty minor, and I can do them myself in a bit if you're feeling tired ;)
Oh, I just thought of this: do you think we should allow this to be configurable? Should a user be able to turn this off, maybe with like http1_only, similar to how there is a http2_only config? It's not needed here, just crossed my mind.
| match self.state.writing { | ||
| Writing::Init => { | ||
| if self.has_h2_prefix() { |
There was a problem hiding this comment.
Oh great idea, so that Http1Transaction doesn't need to get more complicated, I like it!
src/proto/h2/client.rs
Outdated
| @@ -60,6 +60,7 @@ where | |||
| loop { | |||
| let next = match self.state { | |||
| State::Handshaking(ref mut h) => { | |||
| trace!("Polled client future, state is handshaking"); | |||
There was a problem hiding this comment.
I'd probably remove these traces, I'm sure they were useful developing the feature, but afterwards, debugging other things will likely mean these just feel more like noise.
src/server/conn.rs
Outdated
| where | ||
| S: Service, | ||
| T: AsyncRead + AsyncWrite, |
There was a problem hiding this comment.
I believe these bounds can be removed from here and the fmt::Debug impl :)
src/server/conn.rs
Outdated
| proto::h1::Dispatcher< | ||
| proto::h1::dispatch::Server<S>, | ||
| S::ResBody, | ||
| I, | ||
| Rewind<T>, |
There was a problem hiding this comment.
This is just a thought, feel free to tell me it's crazy: it may be possible to actually just pass T here, and not need Rewind<T> only for h1. It seems like the only time rewind is called is when deconstructing h1 and building an h2 dispatcher, so in try_h2 is where it could be let io = Rewind::new(io); io.rewind(read_buf); .... I have a hunch this will mean that this new feature doesn't affect the performance of HTTP1 at all, since the read calls would never need to check the rewind buffer.
There was a problem hiding this comment.
Yeah, this is something I thought about, I cant remember why I didn't do this, let me try to look through the code and jog my memory.
estk
force-pushed
the
try_h2_on_h1_failure
branch
2 times, most recently
from
fa05583
to
839d647
Compare
|
Haha thanks @seanmonstar, this has been an excellent learning experience so I'm not quite ready to give up yet. I've addressed the comments you made today, aside from the Unfortunately it seems like I haven't quite fixed the flakey-ness of the CI tests. It really has me baffled since I cannot reproduce it locally and it seems to happen on different rustc's every time. I've even gone to the trouble of running it in a debian docker container to see if I could replicate it there (no luck.) My suspicion is that it is a real issue, in h2 as you said, but without being able to reproduce it locally its very difficult to debug. If you could point me in the right direction there it would be super helpful. Also, if youre ready to just wrap this up yourself feel free, I'm sure I'll learn a ton from the code you add. |
estk
force-pushed
the
try_h2_on_h1_failure
branch
from
13594f4
to
65a2668
Compare
estk
force-pushed
the
try_h2_on_h1_failure
branch
from
6656175
to
0ec990a
Compare
Closes #1486
TODO: