Made some HTTPHeaders API public, to use in NIOHPACK #525

AlanQuatermain · 2018-07-23T23:49:13Z

There's a public static creator which takes a ByteBuffer and array of HTTPHeader now, and the buffer and headers properties (and thus the HTTPHeader and HTTPHeaderIndex types) are public to support that.

Motivation:

HTTP/2 adds an additional item to a header key/value pair: its indexability. By default, headers can be inserted into the HPACK header table index; sometimes this will be denied (common case is 'content-length' header, which is rarely going to be the same, and will just cause the table to purge more often than is really necessary). Additionally, a header may be marked as not only non-indexable but also non-rewritable by proxies. HTTPHeaders provides nowhere to store this, so the HPACKHeaders implementation referenced in apple/swift-nio-http2#10 was created to add in that capability.

Now, given that we really want the HTTP version to be an implementation detail, we want to keep the HPACK details hidden, and would thus be using HTTPHeaders in client APIs. NIOHTTP2 already has a HTTP1-to-HTTP2 channel handler that translates between the two. Thus it behooves us to have the means to copy the actual key/value pairs between the two types without making a round-trip from UTF-8 bytes to UTF-16 Strings. These changes allow NIOHPACK or NIOHTTP2 to implement that round-trip internally.

Modifications:

HTTPHeader and HTTPHeaderIndex types are now public.
HTTPHeaders.buffer and HTTPHeaders.headers properties are now public.
A new static method, HTTPHeaders.createHeaderBlock(buffer:headers:) was created to serve as a public means to create a HTTPHeaders from a ByteBuffer from outside NIOHTTP1. @Lukasa suggested this should be a static method rather than an initializer.

Result:

Nothing in NIOHTTP1 changes in operation. All public types have documentation comments noting that they are only public for very specific reasons, and are not intended for general use. Once this is committed, NIOHPACK & NIOHTTP2 will be able to implement fast round-trips between HTTPHeaders and HPACKHeaders.

swift-nio-bot · 2018-07-23T23:49:15Z

Can one of the admins verify this patch?

swift-nio-bot · 2018-07-23T23:49:15Z

Can one of the admins verify this patch?

Lukasa

One small note. We'll obviously end up wanting some tests for the new API surface area, but before you write them I want to get @weissi and @normanmaurer to weigh in here and express their opinions as to this API design. A few alternative possibilities that might be suggested:

Don't expose the HTTPHeaderIndex and HTTPHeader types, instead just use list of tuples computed as needed.
Rather than make buffer and headers public, use a with* function to expose them with limited scope and a stern admonition not to escape the buffer.

Lukasa · 2018-07-24T07:26:17Z

Sources/NIOHTTP1/HTTPTypes.swift

    }
+
+    /// This is public to aid in the creation of supplemental HTTP libraries, e.g.
+    /// NIOHTTP2 and NIOHPACK. It is not intended for general use.


I think even though these things aren't for general use, we should explain what they are in the doc comments.

I'd still like an explanation in this function. 😄

I think this comment got lost in the file.

AlanQuatermain · 2018-07-24T18:52:09Z

I like the idea of a with* function. I wasn't entirely certain how to couch things, so I went with the smallest actual API change.

weissi · 2018-07-30T17:55:07Z

thanks @AlanQuatermain! I'm not too fussed about exposing the HTTPHeaderIndex and HTTPHeader types but exposing buffer and headers directly (as properties) I find a little more troublesome. The reason is: If they're properties they should really be constant time and for those two that means our implementation then needs to always back them by a ByteBuffer. For a with* function that's not the case so I think I'd prefer that too.

AlanQuatermain · 2018-07-30T18:29:51Z

Cool, I'll update this to use the with* approach.

Lukasa

Cool, I'm happy with this API. Now all we need is some tests, as well as a quick bit of elaboration on the doc comments.

Lukasa · 2018-07-31T08:57:29Z

Sources/NIOHTTP1/HTTPTypes.swift

    }
+
+    /// This is public to aid in the creation of supplemental HTTP libraries, e.g.
+    /// NIOHTTP2 and NIOHPACK. It is not intended for general use.


I'd still like an explanation in this function. 😄

Lukasa · 2018-07-31T08:58:04Z

Sources/NIOHTTP1/HTTPTypes.swift

+    ///                 characters.
+    ///   - locations:  An array of `HTTPHeader`s, each of which contains information on the location in
+    ///                 the buffer of both a header's name and value.
+    ///   - contiguous: A `Bool` indicating whether the headers are stored contiguously, with no gaps between


Does this render properly? My assumption is that it does not, that these parameter meanings need to be documented in the text for block.

Yep, it does. I had to do some digging to find out the proper layout, but it transpires you just have to give the block parameters names and include then with the method's parameters. They render correctly in a separate table in the block parameter's documentation.

Lukasa · 2018-07-31T08:58:51Z

Sources/NIOHTTP1/HTTPTypes.swift

+    ///   - locations:  An array of `HTTPHeader`s, each of which contains information on the location in
+    ///                 the buffer of both a header's name and value.
+    ///   - contiguous: A `Bool` indicating whether the headers are stored contiguously, with no gaps between
+    ///                 adjacent name/value pairs.


Do you mind elaborating on this last parameter? Specifically, there are gaps, but those gaps correspond to a HTTP/1.1-formatted header block (that is, the gaps are either : or \r\n in the buffer).

Argh, I'd forgotten in contained those. I was thinking of the continuous property that is marked false when something is removed from the buffer, i.e. when the buffer also contains strings that aren't intended to be included in the header list. In other words, whether the buffer can be directly copied.

In retrospect, it doesn't really match my intended use case (i.e. whether HPACKHeaders can just grab the buffer directly without caring about copying extra data—since the HPACK structure isn't supporting the same add/remove functionality and would like a smaller storage load). However, it still seems useful since it can tell receivers if the buffer is already a valid encoded HTTP/1 header block, ready for the wire as-is.

Lukasa

One minor change and then I'm happy.

Lukasa · 2018-08-02T11:14:47Z

Sources/NIOHTTP1/HTTPTypes.swift

    }
+
+    /// This is public to aid in the creation of supplemental HTTP libraries, e.g.
+    /// NIOHTTP2 and NIOHPACK. It is not intended for general use.


I think this comment got lost in the file.

AlanQuatermain · 2018-08-02T18:35:19Z

Le oops. Duly excised 😊

Lukasa · 2018-08-03T08:16:41Z

@swift-nio-bot test this please

Lukasa · 2018-08-03T08:18:42Z

Looks like we're missing the Linux tests. @AlanQuatermain, you may find it useful to place the following file at .git/hooks/pre-commit in the repository:

#!/bin/sh

FIRST_OUT="$(git status --porcelain)"
ruby scripts/generate_linux_tests.rb > /dev/null
NEW_OUT="$(git status --porcelain)"

if [[ "$FIRST_OUT" != "$NEW_OUT" ]]; then
    printf "\033[0;31mMissing linux test changes!\033[0m\n"
    printf "The appropriate changes have been left in the repo.\n"
    printf "Please fix up and then merge.\n"
    exit 1
fi

AlanQuatermain · 2018-08-03T21:31:53Z

@swift-nio-bot test this please

weissi

thanks! That looks good to me now!

weissi · 2018-08-04T13:22:34Z

@swift-nio-bot test this please

@weissi

Motivation: The CONTRIBUTORS file is getting out-of-date quickly, and we forget to update it. We should make updating it part of the contribution flow such that it remains current. I noticed this because it got picked up in apple#525. Modifications: - Added a check to the sanity script to validate the CONTRIBUTORS file is up-to-date. - Updated the mailmap file because @weissi doesn't know how to spell his own name. - Minor clean up in the sanity script to handle the fact it checks three programming languages, so the output is a bit better. Result: Better sanity checking.

Lukasa

One minor note and then I'm happy! ✨

Lukasa · 2018-08-06T09:23:03Z

CONTRIBUTORS.txt

 - Ian Partridge <i.partridge@uk.ibm.com>
 - Jason Toffaletti <toffaletti@gmail.com>
+- Jim Dovey <jimdovey@mac.com>
+- Johannes Weiss <johannesweiss@apple.com>


Let's remove the contributors script, as it has inadvertently added Johannes twice. We should re-run it in a separate PR, and then add it to our sanity check build system to start rejecting builds where the user is not in the contributor mesh.

Ok, see #550 for this. You should leave your own name's entry here in the right place, as that will prevent us hitting a problem if we end up merging #550 before this patch.

Hehe, I was going to suggest the .mailmap thing—I saw the same in the http2 repo, and that the .mailmap file there had two entries for @weissi that were the wrong way round, it seems. I copied his approach first, but then that didn't work so I copied yours 😉

I've simply reverted the Contributors file changes; if the script will be run & inlined with the build or merge process then that should be handled by something other than my commit, I think. I expect Github provides something like a post-pull-request-approval hook that could be used to automate the process?

Oh, wait, I mis-read. So I should update the contributors file but manually delete @weissi's ASCII clone—no problem.

Sorry, no, you were right the first time: let's remove the entire CONTRIBUTORS file change except your entry.

sorry guys 😬. I recently had too much trouble with 'Weiß' so switched to 'Weiss' everywhere 🙃.

Lukasa · 2018-08-06T09:39:11Z

Having @AlanQuatermain on the whitelist should mean we don't have to prompt the bot to build anymore, it should auto-build all of @AlanQuatermain's PRs on this repo.

Lukasa · 2018-08-06T09:39:26Z

Oh wait, he isn't on the whitelist here.

Lukasa · 2018-08-06T09:39:33Z

@swift-nio-bot add to whitelist

@weissi

Motivation: The CONTRIBUTORS file is getting out-of-date quickly, and we forget to update it. We should make updating it part of the contribution flow such that it remains current. I noticed this because it got picked up in apple#525. Modifications: - Added a check to the sanity script to validate the CONTRIBUTORS file is up-to-date. - Updated the mailmap file because @weissi doesn't know how to spell his own name. - Minor clean up in the sanity script to handle the fact it checks three programming languages, so the output is a bit better. Result: Better sanity checking.

Lukasa · 2018-08-08T13:31:35Z

@AlanQuatermain Looks like we have some merge conflicts: can you fix those up and re-push? Then we should be able to land this.

AlanQuatermain · 2018-08-08T18:43:00Z

Face → palm. So much annoying commit replication there. Sigh. If only there were a way to rebase these things cleanly, without generating weird commit histories. I would squash all this if I though that would actually not cause even more problems when I try to push it to my own repo; the pull-request model kinda breaks when you need to have two repositories (local and Github) that need to rebase when there's a merge conflict.

The `HTTPHeaders` API now vends `withUnsafeBufferAndIndices(_:)` and a static factory method to create `HTTPHeaders` from a `ByteBuffer` and a list of `HTTPHeader` values. Tests have been added, and the contributor script manually updated.

AlanQuatermain · 2018-08-08T18:55:25Z

Boom. Damned thing. Made a new branch based on apple/master, merge-squashed my changes onto that, hard-reset master back to 57c6724, then force-pushed.

Single. Commit.

*mic drop*

AlanQuatermain · 2018-08-08T18:57:49Z

For the record, there was a chain of about 50 commits, many of which were identical due to multiple merges/rebases, before the merge-squash created one very small commit containing only the actual changes for this PR. Sigh.

AlanQuatermain mentioned this pull request Jul 23, 2018

Implementation of HPACK encoding and header tables apple/swift-nio-http2#10

Merged

Lukasa requested review from normanmaurer and weissi July 24, 2018 07:23

Lukasa requested changes Jul 24, 2018

View reviewed changes

AlanQuatermain force-pushed the master branch from 5a4e856 to 3b17d6e Compare July 30, 2018 18:37

Lukasa requested changes Jul 31, 2018

View reviewed changes

Lukasa requested changes Aug 2, 2018

View reviewed changes

weissi approved these changes Aug 4, 2018

View reviewed changes

weissi added the 🆕 semver/minor Adds new public API. label Aug 4, 2018

Lukasa mentioned this pull request Aug 6, 2018

Update CONTRIBUTORS and improve workflow. #550

Closed

Lukasa requested changes Aug 6, 2018

View reviewed changes

Lukasa approved these changes Aug 8, 2018

View reviewed changes

Lukasa added this to the 1.10.0 milestone Aug 8, 2018

AlanQuatermain force-pushed the master branch from 20b5344 to 4d6f149 Compare August 8, 2018 18:53

Merge branch 'master' into master

9793e70

Lukasa merged commit 5279d73 into apple:master Aug 9, 2018

Made some HTTPHeaders API public, to use in NIOHPACK #525

Made some HTTPHeaders API public, to use in NIOHPACK #525

Uh oh!

Conversation

AlanQuatermain commented Jul 23, 2018

Motivation:

Modifications:

Result:

Uh oh!

swift-nio-bot commented Jul 23, 2018

Uh oh!

swift-nio-bot commented Jul 23, 2018

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanQuatermain commented Jul 24, 2018

Uh oh!

weissi commented Jul 30, 2018

Uh oh!

AlanQuatermain commented Jul 30, 2018

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanQuatermain commented Aug 2, 2018

Uh oh!

Lukasa commented Aug 3, 2018

Uh oh!

Lukasa commented Aug 3, 2018

Uh oh!

AlanQuatermain commented Aug 3, 2018

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

weissi commented Aug 4, 2018

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa commented Aug 6, 2018

Lukasa commented Aug 6, 2018 •

edited

Loading