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.

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

Improve performance of HTTPHeaders.subscript(canonicalForm:) #1952

Merged
merged 1 commit into from
Sep 13, 2021
Merged
Changes from all commits
Commits
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
33 changes: 21 additions & 12 deletions Sources/NIOHTTP1/HTTPTypes.swift
Original file line number Diff line number Diff line change
Expand Up @@ -540,28 +540,37 @@ extension HTTPHeaders: RandomAccessCollection {
}
}

extension Character {
extension UTF8.CodeUnit {
var isASCIIWhitespace: Bool {
return self == " " || self == "\t" || self == "\r" || self == "\n" || self == "\r\n"
switch self {
case UInt8(ascii: " "),
UInt8(ascii: "\t"),
UInt8(ascii: "\r"),
UInt8(ascii: "\n"):
return true

default:
return false
}
Comment on lines +546 to +554
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently two are missing:

  • Vertical tab \v
  • Form feed \f

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm disinclined to make this more complex. We're only really trying to catch the OWS rule from the HTTP -semantics draft, which strictly forbids newline characters, and I don't think we really expect to see them, so arguably we could even remove the \r and \n cases. VTAB and form-feed should also be disregarded by this loop for the same reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine to me tbh, although it says SHOULD which I'm sure someone somewhere interprets as "not necessary"

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, you can put a VTAB in there: the mistake would be for us to strip it, not for us to fail to. If your VTAB makes it to the app the app is naturally going to flip its lid: what we shouldn't do is treat it as though it was accidentally equivalent to SP or HTAB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we follow up with a PR to remove \r and \n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Or perhaps a good first issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think follow-up, happy to have you do it.

}
}

extension String {
func trimASCIIWhitespace() -> Substring {
return self.dropFirst(0).trimWhitespace()
return Substring(self).trimWhitespace()
}
}

private extension Substring {
func trimWhitespace() -> Substring {
var me = self
while me.first?.isASCIIWhitespace == .some(true) {
me = me.dropFirst()
}
while me.last?.isASCIIWhitespace == .some(true) {
me = me.dropLast()
extension Substring {
fileprivate func trimWhitespace() -> Substring {
guard let firstNonWhitespace = self.utf8.firstIndex(where: { !$0.isASCIIWhitespace }) else {
// The whole substring is ASCII whitespace.
return Substring()
}
return me

// There must be at least one non-ascii whitespace character, so banging here is safe.
let lastNonWhitespace = self.utf8.lastIndex(where: { !$0.isASCIIWhitespace })!
Lukasa marked this conversation as resolved.
Show resolved Hide resolved
return Substring(self.utf8[firstNonWhitespace...lastNonWhitespace])
}
}

Expand Down