Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Aug 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2a267d
More Swift-y types for HTTP/2 frame, flags, and settings.
AlanQuatermain Aug 4, 2018
4876e01
Should have `.complete` in `FrameFlags.allFlags`.
AlanQuatermain Aug 16, 2018
874f7e6
Updated based on PR feedback.
AlanQuatermain Aug 17, 2018
cfde0fc
Fixed a nit.
AlanQuatermain Aug 17, 2018
7732bec
Updated based on PR feedback.
AlanQuatermain Aug 20, 2018
517e3bb
Updated based on PR feedback.
AlanQuatermain Aug 21, 2018
14d2430
Updated based on PR feedback.
AlanQuatermain Aug 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.target(name: "NIOHPACK",
dependencies: ["NIO", "NIOConcurrencyHelpers", "NIOHTTP1"]),
.testTarget(name: "NIOHTTP2Tests",
Expand Down
18 changes: 18 additions & 0 deletions Sources/NIOHTTP2/HTTP2Error.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,24 @@ public enum NIOHTTP2Errors {
self.nghttp2ErrorCode = nghttp2ErrorCode
}
}

/// Received/decoded data was invalid.
public struct InvalidSettings: NIOHTTP2Error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// The network identifier of the setting being read.
public var settingCode: UInt16

/// The offending value.
public var value: Int32

/// The error code associated with the error.
public var errorCode: HTTP2ErrorCode

public init(setting: UInt16, value: Int32, errorCode: HTTP2ErrorCode) {
self.settingCode = setting
self.value = value
self.errorCode = errorCode
}
}
}


158 changes: 93 additions & 65 deletions Sources/NIOHTTP2/HTTP2Frame.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,88 +21,45 @@ public struct HTTP2Frame {
/// The payload of this HTTP/2 frame.
public var payload: FramePayload

/// The frame flags as an 8-bit integer. To set/unset well-defined flags, consider using the
/// other properties on this object (e.g. `endStream`).
public var flags: UInt8
/// The frame flags.
public var flags: FrameFlags

/// The frame stream ID as a 32-bit integer.
public var streamID: HTTP2StreamID

// Whether the END_STREAM flag bit is set.
public var endStream: Bool {
get {
switch self.payload {
case .data, .headers:
return (self.flags & UInt8(NGHTTP2_FLAG_END_STREAM.rawValue) != 0)
default:
return false
private func _hasFlag(_ flag: FrameFlags) -> Bool {
return self.flags.contains(flag)
}

private mutating func _setFlagIfValid(_ flag: FrameFlags) {
if self.payload.allowedFlags.contains(flag) {
self.flags.formUnion(flag)
}
set {
switch self.payload {
case .data, .headers:
self.flags |= UInt8(NGHTTP2_FLAG_END_STREAM.rawValue)
default:
break
}

// Whether the END_STREAM flag bit is set.
public var endStream: Bool {
get { return self._hasFlag(.endStream) }
set { self._setFlagIfValid(.endStream) }
}
}

// Whether the PADDED flag bit is set.
public var padded: Bool {
get {
switch self.payload {
case .data, .headers, .pushPromise:
return (self.flags & UInt8(NGHTTP2_FLAG_PADDED.rawValue) != 0)
default:
return false
}
get { return self._hasFlag(.padded) }
set { self._setFlagIfValid(.padded) }
}
set {
switch self.payload {
case .data, .headers, .pushPromise:
self.flags |= UInt8(NGHTTP2_FLAG_PADDED.rawValue)
default:
break
}
}
}

// Whether the PRIORITY flag bit is set.
public var priority: Bool {
get {
if case .headers = self.payload {
return (self.flags & UInt8(NGHTTP2_FLAG_PRIORITY.rawValue) != 0)
} else {
return false
}
}
set {
if case .headers = self.payload {
self.flags |= UInt8(NGHTTP2_FLAG_PRIORITY.rawValue)
get { return self._hasFlag(.priority) }
set { self._setFlagIfValid(.priority) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;).

}
}
}

// Whether the ACK flag bit is set.
public var ack: Bool {
get {
switch self.payload {
case .settings, .ping:
return (self.flags & UInt8(NGHTTP2_FLAG_ACK.rawValue) != 0)
default:
return false
get { return self._hasFlag(.ack) }
set { self._setFlagIfValid(.ack) }
}
}
set {
switch self.payload {
case .settings, .ping:
self.flags |= UInt8(NGHTTP2_FLAG_ACK.rawValue)
default:
break
}
}
}

public enum FramePayload {
case data(IOData)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case alternativeService(origin: String?, field: ByteBuffer?)
case blocked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think BLOCKED ever got specced.

case origin([String])
case cacheDigest(origin: String?, digest: ByteBuffer?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


var code: UInt8 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a doc comment here?

switch self {
case .data: return 0x0
case .headers: return 0x1
case .priority: return 0x2
case .rstStream: return 0x3
case .settings: return 0x4
case .pushPromise: return 0x5
case .ping: return 0x6
case .goAway: return 0x7
case .windowUpdate: return 0x8
case .continuation: return 0x9
case .alternativeService: return 0xa
case .blocked: return 0xb
case .origin: return 0xc
case .cacheDigest: return 0xd
}
}

var allowedFlags: FrameFlags {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a doc comment here too?

switch self {
case .data:
return [.padded, .endStream]
case .headers:
return [.endStream, .endHeaders, .padded, .priority]
case .pushPromise:
return [.endHeaders, .padded]
case .continuation:
return .endHeaders
case .cacheDigest:
return [.reset, .complete]

case .settings, .ping:
return .ack

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case .priority, .rstStream, .goAway, .windowUpdate,
.alternativeService, .blocked, .origin:
return []
}
}
}

public struct FrameFlags : OptionSet {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: no whitespace before colon.

public typealias RawValue = UInt8

public private(set) var rawValue: UInt8

public init(rawValue: UInt8) {
self.rawValue = rawValue
}

public static let endStream = FrameFlags(rawValue: 0x01)
public static let ack = FrameFlags(rawValue: 0x01)
public static let reset = FrameFlags(rawValue: 0x01)
public static let complete = FrameFlags(rawValue: 0x02)
public static let endHeaders = FrameFlags(rawValue: 0x04)
public static let padded = FrameFlags(rawValue: 0x08)
public static let priority = FrameFlags(rawValue: 0x20)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comments on these too if you please.


// useful for test cases
internal static var allFlags: FrameFlags = [.endStream, .complete, .endHeaders, .padded, .priority]
}
}


internal extension HTTP2Frame {
internal init(streamID: HTTP2StreamID, flags: HTTP2Frame.FrameFlags, payload: HTTP2Frame.FramePayload) {
self.streamID = streamID
self.flags = flags.intersection(payload.allowedFlags)
self.payload = payload
}
internal init(streamID: HTTP2StreamID, flags: UInt8, payload: HTTP2Frame.FramePayload) {
self.streamID = streamID
self.flags = flags
self.flags = FrameFlags(rawValue: flags).intersection(payload.allowedFlags)
self.payload = payload
}
}
Expand All @@ -131,7 +159,7 @@ public extension HTTP2Frame {
/// Constructs a frame header for a given stream ID. All flags are unset.
public init(streamID: HTTP2StreamID, payload: HTTP2Frame.FramePayload) {
self.streamID = streamID
self.flags = 0
self.flags = []
self.payload = payload
}
}
6 changes: 3 additions & 3 deletions Sources/NIOHTTP2/HTTP2Parser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import CNIONghttp2
/// NIO's default settings used for initial settings values on HTTP/2 streams, when the user hasn't
/// overridden that. This limits the max concurrent streams to 100, and limits the max header list
/// size to 16kB, to avoid trivial resource exhaustion on NIO HTTP/2 users.
public let nioDefaultSettings = [
HTTP2Setting(parameter: .maxConcurrentStreams, value: 100),
HTTP2Setting(parameter: .maxHeaderListSize, value: 1<<16)
public let nioDefaultSettings: [HTTP2Setting] = [
.maxConcurrentStreams(100),
.maxHeaderListSize(1<<16)
]


Expand Down
Loading