More Swift-y types for HTTP/2 frame, flags, and settings. #16

AlanQuatermain · 2018-08-16T21:03:10Z

Motivation:

Prior to bringing in my HTTP/2 encoder/decoder, I think my implementations of HTTP/2 frame flags as OptionSets and settings as an enum (i.e. existentials) make for a nicer API, and provide somewhat more information for the compiler to work with when optimizing.

Modifications:

HTTP2Frame.FrameFlags is now an OptionSet, and includes the CACHE_DIGEST reset and complete flag values.
HTTP2Frame.FramePayload now supports all current & proposed HTTP/2 frame types, can return their on-wire identifier, and can return the set of HTTP2Frame.FrameFlags that the frame allows.
HTTP2Setting is now an enum with associated values, and includes the SETTINGS_ACCEPT_CACHE_DIGEST and SETTINGS_ENABLE_CONNECT_PROTOCOL values.
HTTP2Setting already includes code to encode/decode itself, though it's not yet used.
Added NIOHTTP2Errors.InvalidSettings error, used when bad settings are encountered by decoder.
NIOHTTP2 now depends on NIOHPACK.
HTTP2Stream now maintains a HPACKEncoder and HPACKDecoder.
Updated NGHTTP2Session.swift to handle (i.e. fatalError("not implemented")) the additional frame types CONTINUATION, BLOCKED, ORIGIN, and CACHE_DIGEST which were added to HTTP2Frame.FramePayload.
Updated tests to use the new HTTP2Setting type.

Result:

Not a lot changes, beyond the in-memory layout of some things. All these types still have nghttp2 conversion methods, and all tests still run successfully.

Motivation: Prior to bringing in my HTTP/2 encoder/decoder, I think my implementations of HTTP/2 frame flags as `OptionSet`s and settings as an enum (i.e. existentials) make for a nicer API, and provide somewhat more information for the compiler to work with when optimizing. Modifications: - `HTTP2Frame.FrameFlags` is now an `OptionSet`, and includes the CACHE_DIGEST `reset` and `complete` flag values. - `HTTP2Frame.FramePayload` now supports all current & proposed HTTP/2 frame types, can return their on-wire identifier, and can return the set of `HTTP2Frame.FrameFlags` that the frame allows. - `HTTP2Setting` is now an enum with associated values, and includes the SETTINGS_ACCEPT_CACHE_DIGEST and SETTINGS_ENABLE_CONNECT_PROTOCOL values. - `HTTP2Setting` already includes code to encode/decode itself, though it's not yet used. - Added `NIOHTTP2Errors.InvalidSettings` error, used when bad settings are encountered by decoder. - NIOHTTP2 now depends on NIOHPACK. - `HTTP2Stream` now maintains a `HPACKEncoder` and `HPACKDecoder`. - Updated `NGHTTP2Session.swift` to handle (i.e. `fatalError("not implemented")`) the additional frame types CONTINUATION, BLOCKED, ORIGIN, and CACHE_DIGEST which were added to `HTTP2Frame.FramePayload`. - Updated tests to use the new `HTTP2Setting` type. Result: Not a lot changes, beyond the in-memory layout of some things. All these types still have nghttp2 conversion methods, and all tests still run successfully.

ianpartridge · 2018-08-16T21:13:05Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        public static let priority      = FrameFlags(rawValue: 0x20)
+
+        // useful for test cases
+        internal static var allFlags: FrameFlags = [.endStream, .endHeaders, .padded, .priority]


Should this contain complete?

In theory, yes, but in the implementation it's the same value as .ack, so there's not really much value in including it. I could go either way, though.

I must be misunderstanding. .ack is 0x01 and .complete is 0x02?

Er. Yes. Er. Oops. Good catch 😅

Le fixed 😊

AlanQuatermain · 2018-08-16T21:16:23Z

I've more code to come focussed on the actual encode/decode process, but @Lukasa suggested I make smaller pull requests 😉

In here I'm just altering some types to be more swift-y, since once the nghttp2 code is gone we'll be working with them directly much more often thus a nicer API is beneficial. Once this is approved I'll be able to start bringing in my encode/decode flows, part of which you can see in the HTTP2Setting implementation.

One issue I'm running into, though, has to do with the placement of the HPACK encoders/decoders in the HTTPStream type. It looks as though I'd need to take the StreamManager (or something like it) from NGHTTP2Session.swift and bring it into HTTP2Parser.swift, and implement the stream-location and decoding directly within that type. I'm not clear on the behavior of things like the ReentrancyManager though, for instance whether it would be necessary at all once we move away from NGHTTP2. I'm leaning towards renaming HTTP2Parser as NGHTTP2Parser and implementing a fresh one, with the ability to configure either of them into the channel handler stack. I'd appreciate some discussion around the best way to handle this ahead of time, to try and avoid some of the churn that occurred on #10.

Lukasa · 2018-08-17T07:27:59Z

Sources/NIOHTTP2/HTTP2Frame.swift

@@ -114,15 +71,86 @@ public struct HTTP2Frame {
        case ping(HTTP2PingData)
        case goAway(lastStreamID: HTTP2StreamID, errorCode: HTTP2ErrorCode, opaqueData: ByteBuffer?)
        case windowUpdate(windowSizeIncrement: Int)
-        case alternativeService
+        case continuation(HTTPHeaders)


We don't want continuation.

CONTINUATION frames should be completely invisible from the perspective of this object. This is because they don't contain lists of headers, they just contain arbitrary bytes that are smashed together with the previous HEADERS frame. The mental model for this is that CONTINUATION exists literally and only to allow HEADERS to escape from the max frame size limit that applies to other frames.

In this case, we should rightly pretend that this does not exist.

Lukasa

Thanks for this! I like the direction of a lot of this, but have some notes.

Lukasa · 2018-08-17T07:28:28Z

Sources/NIOHTTP2/HTTP2Frame.swift

-        case alternativeService
+        case continuation(HTTPHeaders)
+        case alternativeService(origin: String?, field: ByteBuffer?)
+        case blocked


I don't think BLOCKED ever got specced.

Lukasa · 2018-08-17T07:29:52Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        }
+    }
+
+    public struct FrameFlags : OptionSet {


Nit: no whitespace before colon.

Lukasa · 2018-08-17T07:32:28Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        case alternativeService(origin: String?, field: ByteBuffer?)
+        case blocked
+        case origin([String])
+        case cacheDigest(origin: String?, digest: ByteBuffer?)


For now let's not put in CACHE_DIGEST as it hasn't made it to a full RFC yet.

Lukasa · 2018-08-17T07:35:22Z

Sources/NIOHTTP2/HTTP2Settings.swift

-
-    /// Create a `HTTP2SettingsParameter` that is not known to NIO.
+/// A single setting for HTTP/2, a combination of parameter identifier and its value.
+public enum HTTP2Setting {


Let's not use an enum for this.

I'm already not wild about the use of an enum for FramePayload, but I don't really want to compound the mistake. A Swift enum is entirely non-extensible without performing a breaking release of the software. That means any new specced setting will force a brand new semver major release of this library. That's just not a good experience for anyone involved. It's better to leave this as a struct for now.

Incidentally, the struct may want to be implemented by wrapping a private enum, but that's a separate note.

Wow. 🤦‍♂️ That's… amazingly-not-forward-thinking design of the compiler there. As an API designer by trade, Swift's enums provide a good half of the elegance of Swift for API design. Having them designed in a way that makes them essentially unusable across library boundaries is just… wow. I literally cannot describe just how frustrating and disappointing that is; I read this message just as I was going to bed last night & I couldn't get to sleep for an hour afterwards. It's like "look at all these great things! Oh, you're not working on static binaries? Yeah, you can't use these, sorry."

This basically leaves us with the only (reasonable) option being the rather convoluted model used by ChannelOption in SwiftNIO; I guess now I know why it has that design. Well, I know how to do that, at least, so I'll put something together using that approach, because there has to be something better than 'magic numbers with other magic numbers in a struct'.

Agreed. Take a look at https://github.com/apple/swift-evolution/blob/master/proposals/0192-non-exhaustive-enums.md which aims to solve this for enums in the stdlib and overlays, but not for enums in libraries like SwiftNIO.

Oh FFS, if they only care about fixing it for stdlib & Apple's own frameworks that's just adding insult to injury. Not to mention it severely reducing the usefulness of the Swift 5.0 stable ABI from an third-party library perspective.

@Lukasa Given the problems with enums, would it not make more sense to define frame payloads as protocol/struct types? For example, see https://github.com/AlanQuatermain/H2Swift/blob/1e389199710ace9f6f29795892cbdd0f8dc37232/Sources/H2Swift/Frames/Frame.swift#L193 and https://github.com/AlanQuatermain/H2Swift/blob/master/Sources/H2Swift/Frames/DataFrame.swift

…for the payload types themselves, at least.

For frame payloads I basically propose we leave it alone for now as it's already there, and we take a separate spin at re-designing them later in the process. They should be easily-enough redesigned in isolation and it'll help focus the mind.

Lukasa · 2018-08-17T07:36:17Z

Sources/NIOHTTP2/HTTP2Settings.swift

+    /// encoder can select any size equal to or less than this value by using
+    /// signaling specific to the header compression format inside a header block.
+    /// The initial value is 4,096 octets.
+    case headerTableSize(Int32)


Why are these fields Int32? They should be Int (for programmer convenience).

Lukasa · 2018-08-17T07:37:01Z

Sources/NIOHTTP2/HTTP2Settings.swift

+    /// the Extended CONNECT method when creating new streams, for example to
+    /// bootstrap a WebSocket connection. Receipt of this parameter by a server
+    /// does not have any impact.
+    case enableConnectProtocol(Bool)


Let's not add settings without an RFC number yet.

This actual setting is the entire reason I'm working on HTTP/2 😉

I'm trying to get conforming reference implementations in place to nudge the RFC editor to actually look at the damned thing, since they've been sitting on it for a couple of months now with no action beyond a single question and answer right when it was first put into their queue. The same goes for much of the other items, too: most everything else on HTTP/2 the http-bis WG has completed & submitted to the RFC editors has basically sat dormant with no discussion while those of us who have real actual needs to use it now are holding our breath for it to be published. Hence muggins here trying to come up with a way for our iOS apps to tunnel a WebSocket through an HTTP/2 Adobe Traffic Server proxy to a back-end running a Netty or Akka HTTP/2 server: literally the one use-case that stops us enabling HTTP/2 across the organization. Sigh.

Ultimately I want my branch to be somewhere that I can put together experimental implementations of in-progress standards, to better provide feedback to the IETF & co. WebSocket bootstrapping, origin, and cache digests, however, are things I need to provide an implementation for asap, though, and actually form the primary focus of my work on the codecs and framers. Can you suggest a good way to handle this? Maintaining two parallel branches with this much churn on the implementation details is a real pain, but I guess that's my only option?

Hm, actually, it has an RFC number assigned: RFC8441. It's just waiting on IANA registration of the "websocket" value for the :protocol header: https://www.rfc-editor.org/auth48/rfc8441

This may be a good time to think about what, if anything, the extensibility model should be here. We'd really like to be able to support extensions without requiring that core NIO adopt them directly. In the short term I'm ok with putting this in, so long as we're confident that the various IANA-assigned code points aren't going to change. Given that this has an RFC number we're probably safe.

Actually designing the extension point is a bit tricky, because we have a performance/flexibility trade-off that has to be made.

Lukasa · 2018-08-17T07:38:05Z

Sources/NIOHTTP2/HTTP2Settings.swift

-    public static func ==(lhs: HTTP2Setting, rhs: HTTP2Setting) -> Bool {
-        return lhs.parameter == rhs.parameter && lhs._value == rhs._value
+extension HTTP2Setting {
+    // nullable *and* throws? Invalid data causes an error, but unknown setting types return 'nil' quietly.


We should probably have an escape hatch for unknown settings types, rather than returning nil and throwing them away.

With the non-enum design, that won't be a problem any more: I'll have an 'unknown setting' type to return.

Lukasa · 2018-08-17T07:38:50Z

Sources/NIOHTTP2/HTTP2Stream.swift

+
+    /// An HPACK encoder, handling integer and huffman encoding, header packing, and dynamic header table
+    /// management.
+    var encoder: HPACKEncoder


No need to add these here now, it's just a bunch of memory allocation that the user doesn't need yet.

Lukasa · 2018-08-17T07:39:16Z

Sources/NIOHTTP2/HTTP2Stream.swift

@@ -119,6 +129,8 @@ final class HTTP2Stream {
        self.outboundHeaderStateMachine = HTTP2HeadersStateMachine(mode: mode)
        self.streamID = streamID
        self.dataProvider = HTTP2DataProvider()
+        self.decoder = HPACKDecoder(allocator: ByteBufferAllocator())
+        self.encoder = HPACKEncoder(allocator: ByteBufferAllocator())


As a side note, these allocators should come from the Channel.

Lukasa · 2018-08-17T07:44:36Z

One issue I'm running into, though, has to do with the placement of the HPACK encoders/decoders in the HTTPStream type. It looks as though I'd need to take the StreamManager (or something like it) from NGHTTP2Session.swift and bring it into HTTP2Parser.swift, and implement the stream-location and decoding directly within that type. I'm not clear on the behavior of things like the ReentrancyManager though, for instance whether it would be necessary at all once we move away from NGHTTP2. I'm leaning towards renaming HTTP2Parser as NGHTTP2Parser and implementing a fresh one, with the ability to configure either of them into the channel handler stack. I'd appreciate some discussion around the best way to handle this ahead of time, to try and avoid some of the churn that occurred on #10.

Yeah, you shouldn't try to retrofit your HPACK encoders/decoders onto the HTTP2Parser as it exists today: that object is very definitely nghttp2 specific. Rather than rename it, create a new one with a temporary bad name (e.g. SwiftHTTP2Parser or BetterHTTP2Parser) and work on it alongside the current one: we can then re-use the testing for both before we eventually remove the nghttp2-based one and replace it with the Swift one.

Reentrancy manager exists to defend against the fact that nghttp2 uses callbacks to notify us of events. This is rarely a good idea in a parser because it leads to a number of tricky edge-cases:

You need to know whether the parser is safe to call re-entrantly. nghttp2 doesn't make this clear, so we assume it is not.
You need to ensure that if you call out to code you don't control from these callbacks, that you ensure you will never re-enter in an uncontrolled way.
You need to ensure that you don't screw up feeding data into the parser.

All of this is managed by reentrancy manager essentially turning all recursive re-entrant calls into HTTP2Parser into serial calls instead. For our new parser, we'll want to avoid this risk altogether by changing the parser to avoid using callbacks and instead using return values to trigger callouts. If we use that design, we do not need the re-entrancy manager.

Lukasa

cool, very good very good. Small notes floating around here, but nothing drastic: should be good to merge after these.

Lukasa · 2018-08-20T07:40:26Z

Package.swift

@@ -30,7 +30,7 @@ let package = Package(
        .target(name: "NIOHTTP2Server",
            dependencies: ["NIOHTTP2"]),
        .target(name: "NIOHTTP2",
-            dependencies: ["NIO", "NIOHTTP1", "NIOTLS", "CNIONghttp2"]),
+            dependencies: ["NIO", "NIOHTTP1", "NIOTLS", "CNIONghttp2", "NIOHPACK"]),


We can also probably leave this off for now. Good enthusiasm, but let's not make SwiftPM compile this for no reason.

Lukasa · 2018-08-20T07:41:13Z

Sources/NIOHTTP2/HTTP2Error.swift

@@ -59,6 +59,24 @@ public enum NIOHTTP2Errors {
            self.nghttp2ErrorCode = nghttp2ErrorCode
        }
    }
+
+    /// Received/decoded data was invalid.
+    public struct InvalidSettings: NIOHTTP2Error {


I don't think this struct is actually used anywhere, is it?

Lukasa · 2018-08-20T07:44:00Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        case alternativeService(origin: String?, field: ByteBuffer?)
+        case origin([String])
+
+        var code: UInt8 {


Can we get a doc comment here?

Lukasa · 2018-08-20T07:44:10Z

Sources/NIOHTTP2/HTTP2Frame.swift

+            }
+        }
+
+        var allowedFlags: FrameFlags {


Can we get a doc comment here too?

Lukasa · 2018-08-20T07:44:24Z

Sources/NIOHTTP2/HTTP2Frame.swift

+
+            case .settings, .ping:
+                return .ack
+


There's some surprise whitespace here: mind cleaning it up?

Lukasa · 2018-08-20T07:44:33Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        }
+    }
+
+    public struct FrameFlags: OptionSet {


Doc comment here too please.

Lukasa · 2018-08-20T07:47:50Z

Sources/NIOHTTP2/HTTP2Frame.swift

+        public static let ack           = FrameFlags(rawValue: 0x01)
+        public static let endHeaders    = FrameFlags(rawValue: 0x04)
+        public static let padded        = FrameFlags(rawValue: 0x08)
+        public static let priority      = FrameFlags(rawValue: 0x20)


Doc comments on these too if you please.

Also added copious documentation on HTTP/2 frame payload items, because I'm evil. Mwa-ha-haaa.

Lukasa · 2018-08-21T07:44:11Z

Sources/NIOHTTP2/HTTP2Frame.swift

-            }
-        }
+        get { return self._hasFlag(.priority) }
+        set { self._setFlagIfValid(.priority) }


While we're here, let's ask the question: now that we're using an OptionSet for flags, are these computed properties special enough to stay? @AlanQuatermain, what do you think?

I'd be happy to zap all these, personally. I took the route of least modification for now, though.

There's also the question of what to do if the user wants to set a flag that's invalid for the payload type being used. Silently ignore it? Raise an error? Allow it, but ignore at encode time? Ignore, but raise error when encoding? I've generally gone in the direction of silently ignoring the modification when it's made, in my own implementation.

Hm, that doesn't work with frame.flags.formUnion(someInvalidFlag) though. It looks like we might have use for an internal validateFlags() method or a validatedFlags property that would return the intersection of the set flags and those that are actually allowed on the wire.

Additionally, allowing any flag value to be set means that a client might request padding by setting that flag. I'm in favor of supporting pad bytes when encoding because it's not difficult, but would have it only be used if the client set that flag itself. We wouldn't set it ourselves unless we find a particular use case in testing that would really benefit from it, but really padding seems to be for the benefit of the recipient—our encoding and in-memory representation doesn't cry out for nicely aligned frame sizes. The question, then, is whether the other end does, or whether the intermediary hops would do so. *shrug*

Padding is super weird, it's hard to deploy correctly. We should support serialising it when asked for, but let's not give it a special place in the API: I don't want to encourage it. ;)

Otherwise, I'm fine with adding a didSet that silently strips flags that have no meaning for this frame, or alternatively adding one that traps if you set a bad one.

Oh, whoops, I also forgot to say: let's burn the helpers to the ground.

Oh, and actually let's just ignore bad flags, rather than stripping them or trapping. That gives a logical place for extensions to hook in without us having a really rough time, at least early on: they can just set random flag bits if they want to.

Yeah, I don't want to encourage use of padding, since it's of limited value to us. That said, if someone wants to go setting that flag, we should honour it.

This is a post-hoc comment from me, but I think I disagree with my past self:

Oh, and actually let's just ignore bad flags, rather than stripping them or trapping. That gives a logical place for extensions to hook in without us having a really rough time, at least early on: they can just set random flag bits if they want to.

I think this was a partially thought-out idea. While it's nice for extension hooking, it leads to a substantially less pleasant programming environment for everyone else. Really, the flags are logically part of the payload of a given frame, and we should treat them as such.

This does raise questions about how we're going to handle extensions, but right now we don't have a good extensibility answer and I don't think it's going to land in the 1.0 release (simply no time to solve that issue). So I think we should consider extensions an explicit goal of the 2.0 release, and in the short term build the best API for non-extensible use-cases we can.

This is mostly here as a historical note and to allow @AlanQuatermain to shout me down if he wants ;).

Lukasa · 2018-08-21T07:46:03Z

Sources/NIOHTTP2/NGHTTP2Session.swift

@@ -457,7 +457,7 @@ class NGHTTP2Session {
            }
            nioFramePayload = .settings(settings)
        case NGHTTP2_PUSH_PROMISE.rawValue:
-            nioFramePayload = .pushPromise
+            nioFramePayload = .pushPromise(self.headersAccumulation)


This seems unrelated.

Blech, something didn't get stashed when I switched between branches.

Lukasa

Looks good to me!

ianpartridge reviewed Aug 16, 2018

View reviewed changes

Should have .complete in FrameFlags.allFlags.

4876e01

Lukasa reviewed Aug 17, 2018

View reviewed changes

Lukasa requested changes Aug 17, 2018

View reviewed changes

AlanQuatermain added 2 commits August 17, 2018 14:24

Updated based on PR feedback.

874f7e6

Fixed a nit.

cfde0fc

Lukasa requested changes Aug 20, 2018

View reviewed changes

Updated based on PR feedback.

7732bec

Also added copious documentation on HTTP/2 frame payload items, because I'm evil. Mwa-ha-haaa.

Lukasa reviewed Aug 21, 2018

View reviewed changes

Lukasa requested changes Aug 21, 2018

View reviewed changes

AlanQuatermain added 2 commits August 21, 2018 11:34

Updated based on PR feedback.

517e3bb

Updated based on PR feedback.

14d2430

Lukasa approved these changes Aug 23, 2018

View reviewed changes

Lukasa merged commit f2c2534 into apple:master Aug 23, 2018

More Swift-y types for HTTP/2 frame, flags, and settings. #16

More Swift-y types for HTTP/2 frame, flags, and settings. #16

Conversation

AlanQuatermain commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanQuatermain commented Aug 16, 2018

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Aug 17, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment