Fix up HTTP message framing edge cases. #298

Lukasa · 2018-04-10T17:19:59Z

Motivation:

HTTP message framing has a number of edge cases that NIO currently does
not tolerate. We should decide what our position is on each of these edge
cases and handle it appropriately.

Modifications:

Provide an extensive test suite that codifies our expected handling of
these edge cases. Fix divergences from this behaviour.

Result:

Better tolerance for the weird corners of HTTP.

Resolves #254.

Lukasa · 2018-04-10T17:21:04Z

Sorry about the size of this patch, but FWIW the vast, vast majority of it is a series of tests to clarify our handling of the various edge cases.

normanmaurer · 2018-04-11T07:38:42Z

Sources/NIOHTTP1/HTTPDecoder.swift

+
+                if ((head.status.code / 100 == 1) ||  // 1XX codes
+                    (head.status.code == 204) ||
+                    (head.status.code == 304)) {


nit: You don't need the (...) around rich of these

normanmaurer · 2018-04-11T07:40:23Z

Sources/NIOHTTP1/HTTPDecoder.swift

+        }
+
+        // We don't need the cumulation buffer, if we're holding it.
+        self.cumulationBuffer = nil


@Lukasa should we set this to nil before we call channelReadComplete to reduce the reference count and so minimize the chances of CoW ?

Sure, in fact we can set it to nil very early in this function without danger.

normanmaurer · 2018-04-11T07:41:32Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        case .body:
+            self.seenBody = true
+        case .end:
+            self.seenEnd = true


should we also assert that we not seen the same state before ?

normanmaurer · 2018-04-11T07:43:06Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertTrue(handler.receivedEnd)
+        XCTAssertTrue(handler.eof)
+
+        XCTAssertNoThrow(try channel.finish())


I think we should actually assert the return value of channel.finish()

normanmaurer · 2018-04-11T07:43:26Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertFalse(handler.seenBody)
+        XCTAssert(handler.seenEnd)
+
+        XCTAssertNoThrow(try channel.finish())


I think we should actually assert the return value of channel.finish()

normanmaurer · 2018-04-11T07:43:38Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertFalse(handler.seenBody)
+        XCTAssert(handler.seenEnd)
+
+        XCTAssertNoThrow(try channel.finish())


I think we should actually assert the return value of channel.finish()

normanmaurer · 2018-04-11T07:45:11Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        }
+
+        // Must spin the loop.
+        (channel.eventLoop as! EmbeddedEventLoop).run()


Assert that the channel is not active anymore

normanmaurer · 2018-04-11T07:45:22Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        }
+
+        // Must spin the loop.
+        (channel.eventLoop as! EmbeddedEventLoop).run()


assert the channel is not native anymore.

I think we should actually assert the return value of channel.finish()

Nope, we can't do that here because channel.finish() will throw if the channel is already closed.

normanmaurer · 2018-04-11T07:45:38Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        }
+
+        // Must spin the loop.
+        (channel.eventLoop as! EmbeddedEventLoop).run()


assert the channel is not active anymore

I think we should actually assert the return value of channel.finish()

Nope, we can't do that here because channel.finish() will throw if the channel is already closed.

normanmaurer · 2018-04-11T07:45:59Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertFalse(handler.seenBody)
+        XCTAssert(handler.seenEnd)
+
+        XCTAssertNoThrow(try channel.finish())


assert the return value

normanmaurer · 2018-04-11T07:46:24Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        }
+
+        // Must spin the loop.
+        (channel.eventLoop as! EmbeddedEventLoop).run()


assert that the channel is not active anymore and also call channel.finish() and assert return value.

Nope, we can't do that here because channel.finish() will throw if the channel is already closed.

Lukasa · 2018-04-11T13:15:37Z

Ok @normanmaurer, please re-review.

normanmaurer · 2018-04-11T13:30:08Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+
+/// Tests for the HTTP decoder's handling of message body framing.
+///
+/// Mostly tests assertions in RFC 7230 § 3.3.3.


@Lukasa nit: consider linking the rfc directly via markdown. (feel free to ignore tho)

normanmaurer · 2018-04-11T13:30:45Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertTrue(try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 1, minor: 1),
+                                                                                           method: requestMethod,
+                                                                                           uri: "/"))))
+        _ = channel.readOutbound()


should we ensure this not returns nil ?

normanmaurer

Just one thing... LGTM otherwise

normanmaurer · 2018-04-11T13:31:13Z

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

+        XCTAssertTrue(try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 1, minor: 1),
+                                                                                           method: .GET,
+                                                                                           uri: "/"))))
+        _ = channel.readOutbound()


should we ensure this not return nil ?

Lukasa · 2018-04-11T13:38:46Z

Ok, addressed.

normanmaurer

Nice work!

Lukasa · 2018-04-11T13:59:07Z

@swift-nio-bot test this please

tomerd · 2018-04-11T16:56:20Z

@swift-nio-bot test this please

weissi

thanks so much, this is amazing!!! I left two comments but still approved it but it'd be awesome if you could rename those errno variables.

weissi · 2018-04-12T08:34:07Z

Sources/NIOHTTP1/HTTPDecoder.swift

+        // No check to state.currentError becuase, if we hit it before, we already threw that
+        // error. This never calls any of the callbacks that set that field anyway. Instead we
+        // just check if the errno is set and throw.
+        let errno = parser.http_errno


I'm not a huge fan of naming variables errno because if you rename them and forget the usage sites, it'll still work. Mind making it httpError or httpErrno or something?

weissi · 2018-04-12T08:35:24Z

Sources/NIOHTTP1/HTTPDecoder.swift

@@ -395,7 +416,8 @@ public class HTTPDecoder<HTTPMessageT>: ByteToMessageDecoder, AnyHTTPDecoder {

            let errno = parser.http_errno
            if errno != 0 {
-                throw HTTPParserError.httpError(fromCHTTPParserErrno: http_errno(rawValue: errno))!
+                self.state.currentError = HTTPParserError.httpError(fromCHTTPParserErrno: http_errno(rawValue: errno))!


whilst we're on it, mind also renaming that errno variable up there?

weissi · 2018-04-12T08:37:06Z

@Lukasa compilation failed though:

19:01:45 Linking ./.build/x86_64-unknown-linux/debug/NIOWebSocketServer
19:01:55 /code/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift:155:32: error: value of optional type '[UInt8]?' not unwrapped; did you mean to use '!' or '?'?
19:01:55         XCTAssertEqual(handler.body, Array("some body data".utf8))
19:01:55                                ^
19:01:55                                    !
19:01:55 /code/Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift:166:32: error: value of optional type '[UInt8]?' not unwrapped; did you mean to use '!' or '?'?
19:01:55         XCTAssertEqual(handler.body, Array("some body data".utf8))
19:01:55                                ^
19:01:55                                    !

Motivation: HTTP message framing has a number of edge cases that NIO currently does not tolerate. We should decide what our position is on each of these edge cases and handle it appropriately. Modifications: Provide an extensive test suite that codifies our expected handling of these edge cases. Fix divergences from this behaviour. Result: Better tolerance for the weird corners of HTTP.

Lukasa · 2018-04-12T11:58:18Z

@jw Ok, that should be addressed.

weissi

thanks, approved!

normanmaurer · 2018-04-12T12:22:34Z

@swift-nio-bot test this please

Lukasa added the 🔨 semver/patch No public API change. label Apr 10, 2018

Lukasa requested review from normanmaurer and weissi April 10, 2018 17:20

normanmaurer reviewed Apr 11, 2018

View reviewed changes

normanmaurer requested changes Apr 11, 2018

View reviewed changes

Lukasa force-pushed the cb-http-semantic-eof branch from c8df6c6 to 9364f0c Compare April 11, 2018 13:15

normanmaurer reviewed Apr 11, 2018

View reviewed changes

normanmaurer requested changes Apr 11, 2018

View reviewed changes

Lukasa force-pushed the cb-http-semantic-eof branch from 9364f0c to e3c3bd9 Compare April 11, 2018 13:38

normanmaurer approved these changes Apr 11, 2018

View reviewed changes

weissi approved these changes Apr 12, 2018

View reviewed changes

Lukasa force-pushed the cb-http-semantic-eof branch from e36e55b to 70bd13e Compare April 12, 2018 11:58

weissi approved these changes Apr 12, 2018

View reviewed changes

Merge branch 'master' into cb-http-semantic-eof

6da6f50

Lukasa added this to the 1.5.0 milestone Apr 12, 2018

Lukasa merged commit bc6e3db into apple:master Apr 12, 2018

Lukasa deleted the cb-http-semantic-eof branch April 12, 2018 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix up HTTP message framing edge cases. #298

Fix up HTTP message framing edge cases. #298

Lukasa commented Apr 10, 2018

Lukasa commented Apr 10, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

Lukasa Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

Lukasa Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

Lukasa Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

Lukasa Apr 11, 2018

Lukasa commented Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer Apr 11, 2018

normanmaurer left a comment

normanmaurer Apr 11, 2018

Lukasa commented Apr 11, 2018

normanmaurer left a comment

Lukasa commented Apr 11, 2018

tomerd commented Apr 11, 2018

weissi left a comment

weissi Apr 12, 2018

weissi Apr 12, 2018

weissi commented Apr 12, 2018

Lukasa commented Apr 12, 2018

weissi left a comment

normanmaurer commented Apr 12, 2018

Fix up HTTP message framing edge cases. #298

Fix up HTTP message framing edge cases. #298

Conversation

Lukasa commented Apr 10, 2018

Lukasa commented Apr 10, 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

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 Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Apr 11, 2018

normanmaurer left a comment

Choose a reason for hiding this comment

Lukasa commented Apr 11, 2018

tomerd commented Apr 11, 2018

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi commented Apr 12, 2018

Lukasa commented Apr 12, 2018

weissi left a comment

Choose a reason for hiding this comment

normanmaurer commented Apr 12, 2018