-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of HPACK encoding and header tables #10
Changes from 27 commits
9b435f5
dd514a4
88067f3
b304be4
7aaeb88
6a13d96
d5a36ec
bbfa181
678bbcc
e60f176
c508938
3541b8d
def5662
474a840
ef5f776
a58a1b0
3f0fdda
082a5b3
41602c1
02a3c00
93275b0
f811f14
6afb9be
8e054c6
7e11dd0
0080714
c8e5891
43d8ad5
7e5a322
15e9de9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[submodule "hpack-test-case"] | ||
path = hpack-test-case | ||
url = [email protected]:http2jp/hpack-test-case | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
Johannes Weiß <[email protected]> | ||
<[email protected]> <[email protected]> | ||
<[email protected]> <[email protected]> | ||
<[email protected]> <[email protected]> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,11 @@ needs to be listed here. | |
### Contributors | ||
|
||
- Cory Benfield <[email protected]> | ||
- Jim Dovey <[email protected]> | ||
- Johannes Weiß <[email protected]> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mind making this read
just so my work email address is listed there, sorry |
||
- Johannes Weiß <[email protected]> | ||
- Tim <[email protected]> | ||
- tomer doron <[email protected]> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use my apple email address? [email protected] |
||
|
||
**Updating this list** | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,177 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the SwiftNIO open source project | ||
// | ||
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors | ||
// Licensed under Apache License v2.0 | ||
// | ||
// See LICENSE.txt for license information | ||
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import NIO | ||
|
||
/// Implements the dynamic part of the HPACK header table, as defined in | ||
/// [RFC 7541 § 2.3](https://httpwg.org/specs/rfc7541.html#dynamic.table). | ||
struct DynamicHeaderTable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This structure should get a doc comment. |
||
public static let defaultSize = 4096 | ||
|
||
/// The actual table, with items looked up by index. | ||
private var storage: HeaderTableStorage | ||
|
||
/// The length of the contents of the table. | ||
var length: Int { | ||
return self.storage.length | ||
} | ||
|
||
/// The size to which the dynamic table may currently grow. Represents | ||
/// the current maximum length signaled by the peer via a table-resize | ||
/// value at the start of an encoded header block. | ||
/// | ||
/// - note: This value cannot exceed `self.maximumTableLength`. | ||
var allowedLength: Int { | ||
get { | ||
return self.storage.maxSize | ||
} | ||
set { | ||
self.storage.setTableSize(to: newValue) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than lazily calculate this I'd be inclined to incur this cost eagerly, as it's relatively minor and greatly reduces the cost of the more common header operations. |
||
|
||
/// The maximum permitted size of the dynamic header table as set | ||
/// through a SETTINGS_HEADER_TABLE_SIZE value in a SETTINGS frame. | ||
var maximumTableLength: Int { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this and |
||
didSet { | ||
if self.allowedLength > maximumTableLength { | ||
self.allowedLength = maximumTableLength | ||
} | ||
} | ||
} | ||
|
||
/// The number of items in the table. | ||
var count: Int { | ||
return self.storage.count | ||
} | ||
|
||
init(allocator: ByteBufferAllocator, maximumLength: Int = DynamicHeaderTable.defaultSize) { | ||
self.storage = HeaderTableStorage(allocator: allocator, maxSize: maximumLength) | ||
self.maximumTableLength = maximumLength | ||
self.allowedLength = maximumLength // until we're told otherwise, this is what we assume the other side expects. | ||
} | ||
|
||
/// Subscripts into the dynamic table alone, using a zero-based index. | ||
subscript(i: Int) -> HeaderTableEntry { | ||
return self.storage[i] | ||
} | ||
|
||
func view(of index: HPACKHeaderIndex) -> ByteBufferView { | ||
return self.storage.view(of: index) | ||
} | ||
|
||
// internal for testing | ||
func dumpHeaders() -> String { | ||
return self.storage.dumpHeaders(offsetBy: StaticHeaderTable.count) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we document that our dynamic table is zero indexed for the purposes of this subscript? |
||
|
||
// internal for testing -- clears the dynamic table | ||
mutating func clear() { | ||
self.storage.purge(toRelease: self.storage.length) | ||
} | ||
|
||
/// Searches the table for a matching header, optionally with a particular value. If | ||
/// a match is found, returns the index of the item and an indication whether it contained | ||
/// the matching value as well. | ||
/// | ||
/// Invariants: If `value` is `nil`, result `containsValue` is `false`. | ||
/// | ||
/// - Parameters: | ||
/// - name: The name of the header for which to search. | ||
/// - value: Optional value for the header to find. Default is `nil`. | ||
/// - Returns: A tuple containing the matching index and, if a value was specified as a | ||
/// parameter, an indication whether that value was also found. Returns `nil` | ||
/// if no matching header name could be located. | ||
@_specialize(where Name == String.UTF8View, Value == String.UTF8View) // from String-based API | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we actually need to manually specialise here? We use them both from within this module so it should already be specialised, no? I don't think it's decided yet that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggested it mainly to defend against running into problems. I'm not wedded to it, but it strikes me that we're unlikely to see a meaningful change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lukasa well, syntax might be different. Or the whole same set of problems that we had with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll remove them for now, and they can be re-added if necessary in a performance-focussed update. They caused a crash in the Swift 4.2 compiler anyway… Personally I prefer getting the functionality in place with one commit, then adding the more esoteric performance adjustments separately. First commit works, more can be added for optimization and fine-tuning, of which there may be some here as the HTTP/2 framer implementation evolves. |
||
@_specialize(where Name == ByteBufferView, Value == ByteBufferView) // from HPACKHeaders-based API | ||
func findExistingHeader<Name: Collection, Value: Collection>(named name: Name, value: Value?) -> (index: Int, containsValue: Bool)? where Name.Element == UInt8, Value.Element == UInt8 { | ||
// looking for both name and value, but can settle for just name if no value | ||
// has been provided. Return the first matching name (lowest index) in that case. | ||
guard let value = value else { | ||
// no `first` on AnySequence, just `first(where:)` | ||
return self.storage.firstIndex(matching: name).map { ($0, false) } | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After further discussion I think a lazy This means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the standard library can help us here: return self.headers.enumerated().lazy.filter {
self.buffer.equalCaseInsensitiveASCII(view: name, at: $1.name)
}.map { (idx, header) in idx } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's for return self.headers.lazy.filter { self.buffer.equalCaseInsensitiveASCII(view: name, at: $0.name) } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I've started it, but the Swift 4.2 compiler segfaults when building the unit tests. Sigh. 4.1 seems ok though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Holy crap the new compiler optimizes well. Unit tests with a release build went from running in 14 seconds to 32 when switching to the current release version of Xcode 😮 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, this is all done now, and everything seems happy in shipping Xcode etc. I'll give it another run with the beta toolset and see if I can't get a decent bug report. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, still segfaults while compiling an
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed a bug with the Swift team: SR-8386. |
||
|
||
// If we have a value, locate the index of the lowest header which contains that | ||
// value, but if no value matches, return the index of the lowest header with a | ||
// matching name alone. | ||
var firstNameMatch: Int? = nil | ||
for index in self.storage.indices(matching: name) { | ||
if firstNameMatch == nil { | ||
// record the first (most recent) index with a matching header name, | ||
// in case there's no value match. | ||
firstNameMatch = index | ||
} | ||
|
||
if self.storage.view(of: self.storage[index].value).matches(value) { | ||
// this entry has both the name and the value we're seeking | ||
return (index, true) | ||
} | ||
} | ||
|
||
// no value matches -- but did we find a name? | ||
if let index = firstNameMatch { | ||
return (index, false) | ||
} else { | ||
// no matches at all | ||
return nil | ||
} | ||
} | ||
|
||
/// Appends a header to the table. Note that if this succeeds, the new item's index | ||
/// is always zero. | ||
/// | ||
/// This call may result in an empty table, as per RFC 7541 § 4.4: | ||
/// > "It is not an error to attempt to add an entry that is larger than the maximum size; | ||
/// > an attempt to add an entry larger than the maximum size causes the table to be | ||
/// > emptied of all existing entries and results in an empty table." | ||
/// | ||
/// - Parameters: | ||
/// - name: A collection of UTF-8 code points comprising the name of the header to insert. | ||
/// - value: A collection of UTF-8 code points comprising the value of the header to insert. | ||
/// - Returns: `true` if the header was added to the table, `false` if not. | ||
@_specialize(where Name == String.UTF8View, Value == String.UTF8View) // from String-based API | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment here |
||
mutating func addHeader<Name: Collection, Value: Collection>(named name: Name, value: Value) throws where Name.Element == UInt8, Value.Element == UInt8 { | ||
do { | ||
try self.storage.add(name: name, value: value) | ||
} catch let error as RingBufferError.BufferOverrun { | ||
// ping the error up the stack, with more information | ||
throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + error.amount, | ||
name: name, value: value) | ||
} | ||
} | ||
|
||
/// Appends a header to the table. Note that if this succeeds, the new item's index | ||
/// is always zero. | ||
/// | ||
/// This call may result in an empty table, as per RFC 7541 § 4.4: | ||
/// > "It is not an error to attempt to add an entry that is larger than the maximum size; | ||
/// > an attempt to add an entry larger than the maximum size causes the table to be | ||
/// > emptied of all existing entries and results in an empty table." | ||
/// | ||
/// - Parameters: | ||
/// - name: A contiguous collection of UTF-8 bytes comprising the name of the header to insert. | ||
/// - value: A contiguous collection of UTF-8 bytes comprising the value of the header to insert. | ||
/// - Returns: `true` if the header was added to the table, `false` if not. | ||
@_specialize(where Name == ByteBufferView, Value == ByteBufferView) // from HPACKHeaders-based API | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and here |
||
mutating func addHeader<Name: ContiguousCollection, Value: ContiguousCollection>(nameBytes: Name, valueBytes: Value) throws where Name.Element == UInt8, Value.Element == UInt8 { | ||
do { | ||
try self.storage.add(nameBytes: nameBytes, valueBytes: valueBytes) | ||
} catch let error as RingBufferError.BufferOverrun { | ||
// convert the error to something more useful/meaningful to client code. | ||
throw NIOHPACKErrors.FailedToAddIndexedHeader(bytesNeeded: self.storage.length + error.amount, | ||
name: nameBytes, value: valueBytes) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick question for @normanmaurer . Do we need to add this to
NOTICE.txt
? It's only used for tests.