Skip to content

Commit

Permalink
Add a location to each of 'NIOHTTP2Errors'
Browse files Browse the repository at this point in the history
Motivation:

Without knowing where an error was thrown it can be unnecessarily
difficult to work out the reasons an application ended up in an error
state.

Modifications:

- Add a 'file' string to each of the 'NIOHTTP2Errors' denoting the file
  and line where an error was created
- Add static methods to create errors to 'NIOHTTP2Errors' defaulting
  file and line, this is unfortunately necessary as defaulting file and
  line in the `init` makes the defaulted and non-defaulted errors
  indistinguishable, and the compiler will pick the one with fewest
  defaulted values.
- Add equatable conformance manually so as to ignore 'file'
- Add backing storage to errors which no longer be stored in an existential
  container
- Add 'CustomStringConvertible' conformance to the errors using backing
  storage so that the 'file' is actually visible when converting the
  error
- Deprecate the old `init`s
- Fix all deprecation warnings

Result:

- NIOHTTP2Errors carry a 'file' with them indicating where they were
  created
  • Loading branch information
glbrntt committed Sep 24, 2020
1 parent 28d94c4 commit 0acd091
Show file tree
Hide file tree
Showing 32 changed files with 1,597 additions and 232 deletions.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -228,15 +228,15 @@ struct ConnectionStreamState {
/// Adjusts the stream state to reserve a client stream ID.
mutating func reserveClientStreamID(_ streamID: HTTP2StreamID) throws {
guard self.clientStreamCount < self.maxClientInitiatedStreams else {
throw NIOHTTP2Errors.MaxStreamsViolation()
throw NIOHTTP2Errors.maxStreamsViolation()
}

guard streamID > self.lastClientStreamID else {
throw NIOHTTP2Errors.StreamIDTooSmall()
throw NIOHTTP2Errors.streamIDTooSmall()
}

guard streamID.mayBeInitiatedBy(.client) else {
throw NIOHTTP2Errors.InvalidStreamIDForPeer()
throw NIOHTTP2Errors.invalidStreamIDForPeer()
}

self.lastClientStreamID = streamID
Expand All @@ -246,15 +246,15 @@ struct ConnectionStreamState {
/// Adjusts the stream state to reserve a server stream ID.
mutating func reserveServerStreamID(_ streamID: HTTP2StreamID) throws {
guard self.serverStreamCount < self.maxServerInitiatedStreams else {
throw NIOHTTP2Errors.MaxStreamsViolation()
throw NIOHTTP2Errors.maxStreamsViolation()
}

guard streamID > self.lastServerStreamID else {
throw NIOHTTP2Errors.StreamIDTooSmall()
throw NIOHTTP2Errors.streamIDTooSmall()
}

guard streamID.mayBeInitiatedBy(.server) else {
throw NIOHTTP2Errors.InvalidStreamIDForPeer()
throw NIOHTTP2Errors.invalidStreamIDForPeer()
}

self.lastServerStreamID = streamID
Expand Down Expand Up @@ -310,13 +310,13 @@ struct ConnectionStreamState {
case true where streamID > self.lastClientStreamID,
false where streamID > self.lastServerStreamID:
// The stream in question is idle.
return .connectionError(underlyingError: NIOHTTP2Errors.NoSuchStream(streamID: streamID), type: .protocolError)
return .connectionError(underlyingError: NIOHTTP2Errors.noSuchStream(streamID: streamID), type: .protocolError)
default:
// This stream must have already been closed.
if ignoreClosed {
return .ignoreFrame
return .ignoreFrame
} else {
return .connectionError(underlyingError: NIOHTTP2Errors.NoSuchStream(streamID: streamID), type: .streamClosed)
return .connectionError(underlyingError: NIOHTTP2Errors.noSuchStream(streamID: streamID), type: .streamClosed)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ extension ReceivingGoawayState {
mutating func receiveGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream || lastStreamID == .maxID else {
// The remote peer has sent a GOAWAY with an invalid stream ID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.invalidStreamIDForPeer(), type: .protocolError), effect: nil)
}

let droppedStreams = self.streamState.dropAllStreamsWithIDHigherThan(lastStreamID, droppedLocally: false, initiatedBy: self.role)
Expand All @@ -39,12 +39,12 @@ extension ReceivingGoawayState where Self: RemotelyQuiescingState {
mutating func receiveGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream || lastStreamID == .maxID else {
// The remote peer has sent a GOAWAY with an invalid stream ID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.invalidStreamIDForPeer(), type: .protocolError), effect: nil)
}

if lastStreamID > self.lastLocalStreamID {
// The remote peer has attempted to raise the lastStreamID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.RaisedGoawayLastStreamID(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.raisedGoawayLastStreamID(), type: .protocolError), effect: nil)
}

let droppedStreams = self.streamState.dropAllStreamsWithIDHigherThan(lastStreamID, droppedLocally: false, initiatedBy: self.role)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ extension ReceivingPushPromiseState {
//
// Before any of this, though, we need to check whether the remote peer is even allowed to push!
guard self.peerMayPush else {
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.PushInViolationOfSetting(), type: .protocolError), effect: nil)
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.pushInViolationOfSetting(), type: .protocolError), effect: nil)
}

let validateHeaderBlock = self.headerBlockValidation == .enabled
Expand Down Expand Up @@ -75,7 +75,7 @@ extension ReceivingPushPromiseState where Self: LocallyQuiescingState {
mutating func receivePushPromise(originalStreamID: HTTP2StreamID, childStreamID: HTTP2StreamID, headers: HPACKHeaders) -> StateMachineResultWithEffect {
// This check is duplicated here, because the protocol error of violating this setting is more important than ignoring the frame.
guard self.peerMayPush else {
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.PushInViolationOfSetting(), type: .protocolError), effect: nil)
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.pushInViolationOfSetting(), type: .protocolError), effect: nil)
}

// If we're a client, the server is forbidden from initiating new streams, as we quiesced. However, RFC 7540 wants us to ignore this.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ extension SendingGoawayState {
mutating func sendGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream || lastStreamID == .maxID else {
// The user has sent a GOAWAY with an invalid stream ID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.invalidStreamIDForPeer(), type: .protocolError), effect: nil)
}

let droppedStreams = self.streamState.dropAllStreamsWithIDHigherThan(lastStreamID, droppedLocally: true, initiatedBy: self.peerRole)
Expand All @@ -39,12 +39,12 @@ extension SendingGoawayState where Self: LocallyQuiescingState {
mutating func sendGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream || lastStreamID == .maxID else {
// The user has sent a GOAWAY with an invalid stream ID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.invalidStreamIDForPeer(), type: .protocolError), effect: nil)
}

if lastStreamID > self.lastRemoteStreamID {
// The user has attempted to raise the lastStreamID.
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.RaisedGoawayLastStreamID(), type: .protocolError), effect: nil)
return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.raisedGoawayLastStreamID(), type: .protocolError), effect: nil)
}

// Ok, this is a valid request, so we can now process it like .active.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ extension SendingPushPromiseState {

// First, however, we need to check we can push at all!
guard self.mayPush else {
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.PushInViolationOfSetting(), type: .protocolError), effect: nil)
return StateMachineResultWithEffect(result: .connectionError(underlyingError: NIOHTTP2Errors.pushInViolationOfSetting(), type: .protocolError), effect: nil)
}

do {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ struct HTTP2SettingsState {
/// - onValueChange: A callback that will be invoked once for each setting change.
mutating func receiveSettingsAck(onValueChange: OnValueChangeCallback) throws {
guard self.unacknowlegedSettingsFrames.count > 0 else {
throw NIOHTTP2Errors.ReceivedBadSettings()
throw NIOHTTP2Errors.receivedBadSettings()
}

try self.applySettings(self.unacknowlegedSettingsFrames.removeFirst(), onValueChange: onValueChange)
Expand Down
4 changes: 2 additions & 2 deletions Sources/NIOHTTP2/ContentLengthVerifier.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ extension ContentLengthVerifier {

let newContentLength = expectedContentLength - length
if newContentLength < 0 {
throw NIOHTTP2Errors.ContentLengthViolated()
throw NIOHTTP2Errors.contentLengthViolated()
}
self.expectedContentLength = newContentLength
}
Expand All @@ -42,7 +42,7 @@ extension ContentLengthVerifier {
case .none, .some(0):
break
default:
throw NIOHTTP2Errors.ContentLengthViolated()
throw NIOHTTP2Errors.contentLengthViolated()
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/NIOHTTP2/DOSHeuristics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ extension DOSHeuristics {
}

if self.receivedEmptyDataFrames > self.maximumSequentialEmptyDataFrames {
throw NIOHTTP2Errors.ExcessiveEmptyDataFrames()
throw NIOHTTP2Errors.excessiveEmptyDataFrames()
}
}
}
8 changes: 4 additions & 4 deletions Sources/NIOHTTP2/Frame Buffers/ConcurrentStreamBuffer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ struct ConcurrentStreamBuffer {
// but doesn't match something in the buffer. As a result, it is an error for this frame to have a stream ID lower than or equal to the
// highest stream ID in the buffer: if it did, we should have found it when we searched above. If that constraint is breached, fail the write.
if let lastElement = self.bufferedFrames.last, frame.streamID <= lastElement.streamID {
throw NIOHTTP2Errors.StreamIDTooSmall()
throw NIOHTTP2Errors.streamIDTooSmall()
}

// Ok, the stream ID is fine: buffer this frame.
Expand All @@ -155,7 +155,7 @@ struct ConcurrentStreamBuffer {
// This buffer probably has a HEADERS frame in it, and we really don't want to violate the ordering requirements that implies, so we'll buffer this anyway.
// We still want StreamIDTooSmall protection here.
if frame.streamID <= lastElement.streamID {
throw NIOHTTP2Errors.StreamIDTooSmall()
throw NIOHTTP2Errors.streamIDTooSmall()
}

// Ok, the stream ID is fine: buffer this frame.
Expand Down Expand Up @@ -223,9 +223,9 @@ struct ConcurrentStreamBuffer {
// If we're currently unbuffering this stream, we need to pass the RST_STREAM frame on for correctness. If we aren't, just
// kill it.
if writeBuffer.currentlyUnblocking {
return .forwardAndDrop(writeBuffer.frames, NIOHTTP2Errors.StreamClosed(streamID: frame.streamID, errorCode: reason))
return .forwardAndDrop(writeBuffer.frames, NIOHTTP2Errors.streamClosed(streamID: frame.streamID, errorCode: reason))
} else {
return .succeedAndDrop(writeBuffer.frames, NIOHTTP2Errors.StreamClosed(streamID: frame.streamID, errorCode: reason))
return .succeedAndDrop(writeBuffer.frames, NIOHTTP2Errors.streamClosed(streamID: frame.streamID, errorCode: reason))
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/NIOHTTP2/Frame Buffers/ControlFrameBuffer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ extension ControlFrameBuffer {
guard self.pendingControlFrames.count < self.maximumBufferSize else {
// Appending another frame would violate the maximum buffer size. We're storing too many frames here,
// we gotta move on.
throw NIOHTTP2Errors.ExcessiveOutboundFrameBuffering()
throw NIOHTTP2Errors.excessiveOutboundFrameBuffering()
}
self.pendingControlFrames.append(PendingControlFrame(frame: frame, promise: promise))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ internal struct OutboundFlowControlBuffer {
if !self.streamDataBuffers[frame.streamID].apply({ $0.dataBuffer.bufferWrite((.data(body), promise)) }) {
// We don't have this stream ID. This is an internal error, but we won't precondition on it as
// it can happen due to channel handler misconfiguration or other weirdness. We'll just complain.
throw NIOHTTP2Errors.NoSuchStream(streamID: frame.streamID)
throw NIOHTTP2Errors.noSuchStream(streamID: frame.streamID)
}
return .nothing
case .headers(let headerContent):
Expand Down Expand Up @@ -191,7 +191,7 @@ extension OutboundFlowControlBuffer {
// RFC 7540 § 5.3, where we can, so this hook is already in place for us to extend later.
if streamID == priorityData.dependency {
// Streams may not depend on themselves!
throw NIOHTTP2Errors.PriorityCycle(streamID: streamID)
throw NIOHTTP2Errors.priorityCycle(streamID: streamID)
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions Sources/NIOHTTP2/HPACKHeaders+Validation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ fileprivate enum BlockSection {
case (.headers, .pseudoHeaderField):
// This is an error: it's not allowed to send a pseudo-header field once a regular
// header field has been sent.
throw NIOHTTP2Errors.PseudoHeaderAfterRegularHeader(":\(field.baseName)")
throw NIOHTTP2Errors.pseudoHeaderAfterRegularHeader(":\(field.baseName)")
}
}
}
Expand Down Expand Up @@ -97,7 +97,7 @@ extension HeaderBlockValidator {
// and at least the mandatory set.
guard validator.allowedPseudoHeaderFields.isSuperset(of: seenPseudoHeaders) &&
validator.mandatoryPseudoHeaderFields.isSubset(of: seenPseudoHeaders) else {
throw NIOHTTP2Errors.InvalidPseudoHeaders(block)
throw NIOHTTP2Errors.invalidPseudoHeaders(block)
}
}
}
Expand Down Expand Up @@ -153,7 +153,7 @@ extension RequestBlockValidator: HeaderBlockValidator {
case .path:
// This is a path pseudo-header. It must not be empty.
if value.utf8.count == 0 {
throw NIOHTTP2Errors.EmptyPathHeader()
throw NIOHTTP2Errors.emptyPathHeader()
}
default:
break
Expand All @@ -163,7 +163,7 @@ extension RequestBlockValidator: HeaderBlockValidator {

// We want to check that if the TE header field is present, it only contains "trailers".
if name.baseName == "te" && value != "trailers" {
throw NIOHTTP2Errors.ForbiddenHeaderField(name: String(name.baseName), value: value)
throw NIOHTTP2Errors.forbiddenHeaderField(name: String(name.baseName), value: value)
}
}
}
Expand Down Expand Up @@ -253,7 +253,7 @@ extension HeaderFieldName {
}

guard baseNameBytes.isValidFieldName else {
throw NIOHTTP2Errors.InvalidHTTP2HeaderFieldName(fieldName)
throw NIOHTTP2Errors.invalidHTTP2HeaderFieldName(fieldName)
}
}

Expand All @@ -272,7 +272,7 @@ extension HeaderFieldName {

switch self.baseName {
case "connection", "transfer-encoding", "proxy-connection":
throw NIOHTTP2Errors.ForbiddenHeaderField(name: String(self.baseName), value: value)
throw NIOHTTP2Errors.forbiddenHeaderField(name: String(self.baseName), value: value)
default:
return
}
Expand Down Expand Up @@ -382,11 +382,11 @@ extension PseudoHeaders {
}

guard let pseudoHeaderType = PseudoHeaders(headerFieldName: name) else {
throw NIOHTTP2Errors.UnknownPseudoHeader(":\(name.baseName)")
throw NIOHTTP2Errors.unknownPseudoHeader(":\(name.baseName)")
}

if self.contains(pseudoHeaderType) {
throw NIOHTTP2Errors.DuplicatePseudoHeader(":\(name.baseName)")
throw NIOHTTP2Errors.duplicatePseudoHeader(":\(name.baseName)")
}

self.formUnion(pseudoHeaderType)
Expand Down
Loading

0 comments on commit 0acd091

Please sign in to comment.