Add support for IO body (for websockets) #134
Conversation
| @@ -16,6 +16,13 @@ const LF: u8 = b'\n'; | |||
| /// The number returned from httparse when the request is HTTP 1.1 | |||
| const HTTP_1_1_VERSION: u8 = 1; | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub(crate) enum BodyType { | |||
There was a problem hiding this comment.
Should we keep this to u32 rather than 2 usize since browsers only supports 2GB content length?
pub(crate) enum BodyType {
FixedLength(NonZeroU32),
Chunked,
}There was a problem hiding this comment.
I don't think the assumption that the client is a mainstream browser should be baked into the library.
There was a problem hiding this comment.
Okay, but what if we make it NonZeroUsize? I think it is quite wasteful to use one usize for only one bit.
There was a problem hiding this comment.
There are three variants anyway. Even if there weren't it corresponds to the Content-Length header which can be 0.
There was a problem hiding this comment.
Oh, I didn't know Content-Length can be zero. But I don't understand why BodyType have Close, if it is closed means there are no body right?
There was a problem hiding this comment.
no, it means the end of the body is indicated by the connection being closed. in http terms if the content length isn't specified and the transfer encoding isn't chunked.
no body is a separate concern, either the connection is closed without sending more data, content-length is set to 0 or transfer encoding is chunked and a 0 size chunk is sent
| let mut buf: Vec<u8> = Vec::new(); | ||
| buf.resize(8 * 1024, 0); |
There was a problem hiding this comment.
Why not use with_capacity? And tweak the logic below a bit.
| let mut buf: Vec<u8> = Vec::new(); | |
| buf.resize(8 * 1024, 0); | |
| let mut buf: Vec<u8> = Vec::with_capacity(8 * 1024); |
There was a problem hiding this comment.
with_capacity makes sense, I'm not sure what the tweak you're referring to is.
There was a problem hiding this comment.
Well, with_capacity only reserve the capacity but it does not fill it up. I think you might want this instead, I am not sure if the _u8 can be elided.
| let mut buf: Vec<u8> = Vec::new(); | |
| buf.resize(8 * 1024, 0); | |
| let mut buf = [0_u8].repeat(8 * 1024); |
There was a problem hiding this comment.
It looks like repeat might be more optimized here. That should work fine either way. It's worth noting that I haven't benchmarked this code at all so I have no idea if it even performs decently in the first place.
| } | ||
|
|
||
| for idx in 3..n { | ||
| if idx >= 3 && &buf[idx - 3..=idx] == b"\r\n\r\n" { |
There was a problem hiding this comment.
Do we need this check?
| if idx >= 3 && &buf[idx - 3..=idx] == b"\r\n\r\n" { | |
| if &buf[idx - 3..=idx] == b"\r\n\r\n" { |
| } | ||
| } | ||
|
|
||
| if pos == buf.len() { |
There was a problem hiding this comment.
| if pos == buf.len() { | |
| if pos == buf_len { |
|
|
||
| let mut read_failure = None; | ||
|
|
||
| // read more if no body has been recieved |
There was a problem hiding this comment.
| // read more if no body has been recieved | |
| // read more if no body has been received |
| Ok(n) => { | ||
| n | ||
| } |
There was a problem hiding this comment.
| Ok(n) => { | |
| n | |
| } | |
| Ok(n) => n, |
There was a problem hiding this comment.
in general, this needs rustfmt run on the whole pr. i've mentioned this before to @Yarn
| } | ||
| }; | ||
|
|
||
| match io_send.send(Ok((buf_ready.clone(), n))).await { |
There was a problem hiding this comment.
I can't find the receiver end for this but do we need to send if n == 0? Or is it just to signal read is done?
There was a problem hiding this comment.
It's to signal that it's done. io_send is set to one of the two senders on line 224 https://github.com/http-rs/async-h1/pull/134/files/d5585c722124f3d819c100d662016125dffff0cb#diff-afb1712a59f738bc3dfe39004cb5e4b9R224
|
closing this — upgrades are in async-h1 2.2.0 |
http-rs/tide#67
http-rs/http-types#231