diff --git a/src/hpack/encoder.rs b/src/hpack/encoder.rs index 2fc72ae..24c186a 100644 --- a/src/hpack/encoder.rs +++ b/src/hpack/encoder.rs @@ -9,6 +9,7 @@ pub struct Encoder { size_update: Option, } +#[derive(Debug)] pub enum EncoderError { } @@ -166,7 +167,9 @@ fn encode_str(val: &[u8], dst: &mut BytesMut) { // Shift the header forward for i in 0..huff_len { - dst[idx + head_len + (huff_len - i)] = dst[idx + 1 + (huff_len - 1)]; + let src_i = idx + 1 + (huff_len - (i+1)); + let dst_i = idx + head_len + (huff_len - (i+1)); + dst[dst_i] = dst[src_i]; } // Copy in the head @@ -325,7 +328,7 @@ mod test { for i in 1..65 { let key = format!("x-hello-world-{:02}", i); let res = encode(&mut encoder, vec![header(&key, &key)]); - assert_eq!(0x80 | (i + 61), res[0]); + assert_eq!(0x80 | (61 + (65-i)), res[0]); } } @@ -419,11 +422,11 @@ mod test { // Encode the first one again let res = encode(&mut encoder, vec![header(name, "one")]); - assert_eq!(&[0x80 | 62], &res[..]); + assert_eq!(&[0x80 | 63], &res[..]); // Now the second one let res = encode(&mut encoder, vec![header(name, "two")]); - assert_eq!(&[0x80 | 63], &res[..]); + assert_eq!(&[0x80 | 62], &res[..]); } #[test] @@ -440,12 +443,12 @@ mod test { // This will evict the first header, while still referencing the header // name let res = encode(&mut encoder, vec![header("foo", "baz")]); - assert_eq!(&[0x40 | 62, 0x80 | 3], &res[..2]); + assert_eq!(&[0x40 | 63, 0, 0x80 | 3], &res[..3]); assert_eq!(2, encoder.table.len()); // Try adding the same header again let res = encode(&mut encoder, vec![header("foo", "baz")]); - assert_eq!(&[0x80 | 63], &res[..]); + assert_eq!(&[0x80 | 62], &res[..]); assert_eq!(2, encoder.table.len()); } @@ -547,6 +550,10 @@ mod test { // Test hitting end at multiple points. } + #[test] + fn test_evicted_overflow() { + // Not sure what the best way to do this is. + } fn encode(e: &mut Encoder, hdrs: Vec
) -> BytesMut { let mut dst = BytesMut::with_capacity(1024); diff --git a/src/hpack/header.rs b/src/hpack/header.rs index 9d033bd..02eb142 100644 --- a/src/hpack/header.rs +++ b/src/hpack/header.rs @@ -6,7 +6,7 @@ use http::header::{HeaderName, HeaderValue}; use bytes::Bytes; /// HTTP/2.0 Header -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] pub enum Header { Field { name: HeaderName, diff --git a/src/hpack/table.rs b/src/hpack/table.rs index fdfd720..cd19393 100644 --- a/src/hpack/table.rs +++ b/src/hpack/table.rs @@ -12,9 +12,7 @@ pub struct Table { mask: usize, indices: Vec>, slots: VecDeque, - // This tracks the number of evicted elements. It is expected to wrap. This - // value is used to map `Pos::index` to the actual index in the VecDeque. - evicted: usize, + inserted: usize, // Size is in bytes size: usize, max_size: usize, @@ -77,7 +75,7 @@ impl Table { mask: 0, indices: vec![], slots: VecDeque::new(), - evicted: 0, + inserted: 0, size: 0, max_size: max_size, } @@ -90,7 +88,7 @@ impl Table { mask: capacity.wrapping_sub(1), indices: vec![None; capacity], slots: VecDeque::with_capacity(usable_capacity(capacity)), - evicted: 0, + inserted: 0, size: 0, max_size: max_size, } @@ -113,6 +111,9 @@ impl Table { // Don't index certain headers. This logic is borrowed from nghttp2. if header.skip_value_index() { + // Right now, if this is true, the header name is always in the + // static table. At some point in the future, this might not be true + // and this logic will need to be updated. return Index::new(statik, header); } @@ -155,7 +156,7 @@ impl Table { // displacement. let their_dist = probe_distance(self.mask, pos.hash, probe); - let slot_idx = pos.index.wrapping_sub(self.evicted); + let slot_idx = pos.index.wrapping_add(self.inserted); if their_dist < dist { // Index robinhood @@ -184,7 +185,7 @@ impl Table { // capacity. loop { // Compute the real index into the VecDeque - let real_idx = index.wrapping_sub(self.evicted); + let real_idx = index.wrapping_add(self.inserted); if self.slots[real_idx].header.value_eq(&header) { // We have a full match! @@ -200,34 +201,25 @@ impl Table { return Index::Name(real_idx + DYN_OFFSET, header); } - self.update_size(header.len(), index); + self.update_size(header.len(), Some(index)); - let new_idx = self.slots.len(); + // Insert the new header + self.insert(header, hash); - // If `evicted` is greater than `index`, then the previous node in - // the linked list got evicted. The header we are about to insert is - // the new "head" of the list and `indices` has already been - // updated. So, all that is left to do is insert the header in the - // VecDeque. - // - // TODO: This logic isn't correct in the face of wrapping - if self.evicted <= index { - // Recompute `real_idx` since this could have been modified by - // entries being evicted - let real_idx = index.wrapping_sub(self.evicted); + // Recompute real_idx as it just changed. + let new_real_idx = index.wrapping_add(self.inserted); - self.slots[real_idx].next = Some(new_idx.wrapping_add(self.evicted)); + // The previous node in the linked list may have gotten evicted + // while making room for this header. + if new_real_idx < self.slots.len() { + let idx = 0usize.wrapping_sub(self.inserted); + + self.slots[new_real_idx].next = Some(idx); } - self.slots.push_back(Slot { - hash: hash, - header: header, - next: None, - }); - // Even if the previous header was evicted, we can still reference // it when inserting the new one... - return Index::InsertedValue(real_idx + DYN_OFFSET, &self.slots[new_idx].header); + return Index::InsertedValue(real_idx + DYN_OFFSET, &self.slots[0].header); } Index::NotIndexed(header) @@ -247,7 +239,7 @@ impl Table { // Passing in `usize::MAX` for prev_idx since there is no previous // header in this case. - if self.update_size(header.len(), usize::MAX) { + if self.update_size(header.len(), None) { if dist != 0 { let back = probe.wrapping_sub(1) & self.mask; @@ -263,15 +255,9 @@ impl Table { } } - // The index is offset by the current # of evicted elements - let slot_idx = self.slots.len(); - let pos_idx = slot_idx.wrapping_add(self.evicted); + self.insert(header, hash); - self.slots.push_back(Slot { - hash: hash, - header: header, - next: None, - }); + let pos_idx = 0usize.wrapping_sub(self.inserted); let mut prev = mem::replace(&mut self.indices[probe], Some(Pos { index: pos_idx, @@ -293,12 +279,22 @@ impl Table { } if let Some((n, _)) = statik { - Index::InsertedValue(n, &self.slots[slot_idx].header) + Index::InsertedValue(n, &self.slots[0].header) } else { - Index::Inserted(&self.slots[slot_idx].header) + Index::Inserted(&self.slots[0].header) } } + fn insert(&mut self, header: Header, hash: HashValue) { + self.inserted = self.inserted.wrapping_add(1); + + self.slots.push_front(Slot { + hash: hash, + header: header, + next: None, + }); + } + pub fn resize(&mut self, size: usize) { self.max_size = size; @@ -310,18 +306,18 @@ impl Table { } self.slots.clear(); - self.evicted = 0; + self.inserted = 0; } else { - self.converge(usize::MAX); + self.converge(None); } } - fn update_size(&mut self, len: usize, prev_idx: usize) -> bool { + fn update_size(&mut self, len: usize, prev_idx: Option) -> bool { self.size += len; self.converge(prev_idx) } - fn converge(&mut self, prev_idx: usize) -> bool { + fn converge(&mut self, prev_idx: Option) -> bool { let mut ret = false; while self.size > self.max_size { @@ -332,19 +328,16 @@ impl Table { ret } - fn evict(&mut self, prev_idx: usize) { - debug_assert!(!self.slots.is_empty()); + fn evict(&mut self, prev_idx: Option) { + let pos_idx = (self.slots.len() - 1).wrapping_sub(self.inserted); // Remove the header - let slot = self.slots.pop_front().unwrap(); + let slot = self.slots.pop_back().unwrap(); let mut probe = desired_pos(self.mask, slot.hash); // Update the size self.size -= slot.header.len(); - // Equivalent to 0.wrapping_add(self.evicted); - let pos_idx = self.evicted; - // Find the associated position probe_loop!(probe < self.indices.len(), { let mut pos = self.indices[probe].unwrap(); @@ -353,8 +346,8 @@ impl Table { if let Some(idx) = slot.next { pos.index = idx; self.indices[probe] = Some(pos); - } else if pos.index == prev_idx { - pos.index = (self.slots.len() + 1).wrapping_add(self.evicted); + } else if Some(pos.index) == prev_idx { + pos.index = 0usize.wrapping_sub(self.inserted + 1); self.indices[probe] = Some(pos); } else { self.indices[probe] = None; @@ -364,8 +357,6 @@ impl Table { break; } }); - - self.evicted = self.evicted.wrapping_add(1); } // Shifts all indices that were displaced by the header that has just been diff --git a/src/hpack/test.rs b/src/hpack/test.rs index 28a673e..94e09b1 100644 --- a/src/hpack/test.rs +++ b/src/hpack/test.rs @@ -1,10 +1,12 @@ +extern crate bytes; extern crate hex; extern crate walkdir; extern crate serde; extern crate serde_json; -use super::{Header, Decoder}; +use super::{Header, Decoder, Encoder}; +use self::bytes::BytesMut; use self::hex::FromHex; use self::serde_json::Value; use self::walkdir::WalkDir; @@ -37,6 +39,14 @@ fn hpack_fixtures() { } } + println!(""); + println!(""); + println!("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"); + println!("~~~ {:?} ~~~", entry.path()); + println!("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"); + println!(""); + println!(""); + test_fixture(entry.path()); } } @@ -86,6 +96,7 @@ fn test_story(story: Value) { let mut decoder = Decoder::default(); + // First, check decoding against the fixtures for case in &cases { let mut expect = case.expect.clone(); @@ -101,6 +112,31 @@ fn test_story(story: Value) { assert_eq!(0, expect.len()); } + + let mut encoder = Encoder::default(); + let mut decoder = Decoder::default(); + + // Now, encode the headers + for case in &cases { + let mut buf = BytesMut::with_capacity(64 * 1024); + + if let Some(size) = case.header_table_size { + encoder.update_max_size(size); + decoder.queue_size_update(size); + } + + let mut input: Vec<_> = case.expect.iter().map(|&(ref name, ref value)| { + Header::new(name.clone().into(), value.clone().into()).unwrap() + }).collect(); + + encoder.encode(input.clone(), &mut buf).unwrap(); + + decoder.decode(&buf.into(), |e| { + assert_eq!(e, input.remove(0)); + }).unwrap(); + + assert_eq!(0, input.len()); + } } }