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

Add support for automatic HTTP error reporting. #268

Merged
merged 6 commits into from Apr 7, 2018

Conversation

Projects
None yet
4 participants
@Lukasa
Copy link
Contributor

Lukasa commented Apr 3, 2018

Motivation:

Currently the HTTP decoders can throw errors, but they will be ignored
and lead to a simple EOF. That's not ideal: in most cases we should make
a best-effort attempt to send a 4XX error code before we shut the client
down.

Modifications:

Provided a new ChannelHandler that generates 400 errors when the HTTP
decoder fails.
Added a flag to automatically add that handler to the channel pipeline.
Added the handler to the HTTP sample server.
Enabled integration test 12.

Result:

Easier error handling for HTTP servers.

@Lukasa Lukasa requested review from normanmaurer and weissi Apr 3, 2018

@Lukasa Lukasa force-pushed the Lukasa:cb-fix-integration-tests branch from 265104a to 3f06fc1 Apr 3, 2018

@weissi

weissi approved these changes Apr 3, 2018

Copy link
Contributor

weissi left a comment

that's great, thank you!

@weissi

This comment has been minimized.

Copy link
Contributor

weissi commented Apr 3, 2018

CC'ing @tanner0101 because they probably want to use the withErrorReporting: flag in Vapor, not critical of course.

@weissi

This comment has been minimized.

Copy link
Contributor

weissi commented Apr 3, 2018

@Lukasa hmm, this hit

14:29:55 Test Case 'ChannelTests.testWeDontCrashIfChannelReleasesBeforePipeline' started at 2018-04-03 12:29:55.569
14:29:55 /code/Tests/NIOTests/ChannelTests.swift:1388: error: ChannelTests.testWeDontCrashIfChannelReleasesBeforePipeline : XCTAssertNil failed: "SocketChannel { selectable = BaseSocket { fd=-1 } , localAddress = nil, remoteAddress = nil }" - weakClientChannel not nil, looks like we leaked it!

looks like we have a race in that test, this shouldn't be related to your changes AFAIK

@Lukasa

This comment has been minimized.

Copy link
Contributor Author

Lukasa commented Apr 3, 2018

@swift-nio-bot test this please

@Lukasa Lukasa force-pushed the Lukasa:cb-fix-integration-tests branch from 2b70cb4 to 48e833f Apr 3, 2018

Add support for automatic HTTP error reporting.
Motivation:

Currently the HTTP decoders can throw errors, but they will be ignored
and lead to a simple EOF. That's not ideal: in most cases we should make
a best-effort attempt to send a 4XX error code before we shut the client
down.

Modifications:

Provided a new ChannelHandler that generates 400 errors when the HTTP
decoder fails.
Added a flag to automatically add that handler to the channel pipeline.
Added the handler to the HTTP sample server.
Enabled integration test 12.

Result:

Easier error handling for HTTP servers.

@Lukasa Lukasa force-pushed the Lukasa:cb-fix-integration-tests branch from 48e833f to bc7e1c8 Apr 3, 2018

@Lukasa Lukasa added this to the 1.4.0 milestone Apr 4, 2018

let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
let head = HTTPResponseHead(version: .init(major: 1, minor: 1), status: .badRequest, headers: headers)
ctx.write(self.wrapOutboundOut(.head(head)), promise: nil)
ctx.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)

This comment has been minimized.

@normanmaurer

normanmaurer Apr 4, 2018

Member

@Lukasa should we also attach a callback to the future which closes the channel once written ?

This comment has been minimized.

@Lukasa

Lukasa Apr 5, 2018

Author Contributor

No. Any of these errors will cause a channel to be closed by the HTTPDecoder, so I don't think it's required.

@normanmaurer
Copy link
Member

normanmaurer left a comment

Just a suggestion

// A side note here: we cannot block or do any delayed work. ByteToMessageDecoder is going
// to come along and close the channel right after we return from this function.
let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
let head = HTTPResponseHead(version: .init(major: 1, minor: 1), status: .badRequest, headers: headers)

This comment has been minimized.

@normanmaurer

normanmaurer Apr 4, 2018

Member

is 1 .1 a good choice here or should we use 1.0 ?

This comment has been minimized.

@Lukasa

Lukasa Apr 5, 2018

Author Contributor

What value do we obtain by using 1.0?

This comment has been minimized.

@normanmaurer

normanmaurer Apr 5, 2018

Member

Was just wondering if 1.0 would be a better choice but I guess 1.1 is more used these days so all good

@tomerd

This comment has been minimized.

Copy link
Member

tomerd commented Apr 5, 2018

@swift-nio-bot test this please

@tomerd

This comment has been minimized.

Copy link
Member

tomerd commented Apr 5, 2018

@Lukasa @weissi leak detector tripped

00:02:29.505 Fatal error: leaking promise created at (file: "/code/Sources/NIO/ChannelInvoker.swift", line: 171): file /code/Sources/NIO/ChannelInvoker.swift, line 171
00:02:29.505 Current stack trace:
00:02:29.506 0    libswiftCore.so                    0x00007f81708bcf90 _swift_stdlib_reportFatalErrorInFile + 221
00:02:29.506 1    libswiftCore.so                    0x00007f81705c6330 closure #1 in closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) + 284
00:02:29.507 2    libswiftCore.so                    0x00007f81708669a1 <unavailable> + 4155809
00:02:29.507 3    libswiftCore.so                    0x00007f817087d059 <unavailable> + 4247641
00:02:29.508 4    libswiftCore.so                    0x00007f81705c5c0b <unavailable> + 1399819
00:02:29.508 5    libswiftCore.so                    0x00007f81707df6b9 <unavailable> + 3602105
00:02:29.509 6    libswiftCore.so                    0x00007f817081d79f <unavailable> + 3856287
00:02:29.509 7    libswiftCore.so                    0x00007f81705c5c0b <unavailable> + 1399819
00:02:29.510 8    libswiftCore.so                    0x00007f817075f7a0 specialized _assertionFailure(_:_:file:line:flags:) + 144
00:02:29.510 9    swift-nioPackageTests.xctest       0x00000000004cadbc <unavailable> + 830908
00:02:29.511 10   swift-nioPackageTests.xctest       0x00000000004caf1c <unavailable> + 831260
00:02:29.511 11   swift-nioPackageTests.xctest       0x000000000056c72c <unavailable> + 1492780
00:02:29.512 12   swift-nioPackageTests.xctest       0x000000000056c62d <unavailable> + 1492525
00:02:29.512 13   swift-nioPackageTests.xctest       0x00000000004ca961 <unavailable> + 829793
00:02:29.513 14   swift-nioPackageTests.xctest       0x00000000004caf4f <unavailable> + 831311
00:02:29.513 15   libswiftCore.so                    0x00007f81708841cb <unavailable> + 4276683
00:02:29.514 16   swift-nioPackageTests.xctest       0x000000000049b34f <unavailable> + 635727
00:02:29.514 17   libswiftCore.so                    0x00007f81708841cb <unavailable> + 4276683
00:02:29.515 18   swift-nioPackageTests.xctest       0x00000000004c2749 <unavailable> + 796489
00:02:29.515 19   swift-nioPackageTests.xctest       0x00000000004c2771 <unavailable> + 796529
00:02:29.516 20   libswiftCore.so                    0x00007f81708841cb <unavailable> + 4276683
00:02:29.516 21   libswiftCore.so                    0x00007f8170885a5c <unavailable> + 4282972
00:02:29.517 22   libswiftCore.so                    0x00007f8170605740 _ContiguousArrayStorage.__deallocating_deinit + 57
00:02:29.517 23   libswiftCore.so                    0x00007f81708841cb <unavailable> + 4276683
00:02:29.518 24   swift-nioPackageTests.xctest       0x00000000004bcb0a <unavailable> + 772874
00:02:29.518 25   swift-nioPackageTests.xctest       0x00000000004bc779 <unavailable> + 771961
00:02:29.519 26   swift-nioPackageTests.xctest       0x00000000004c0078 <unavailable> + 786552
00:02:29.519 27   swift-nioPackageTests.xctest       0x00000000004c4f0c <unavailable> + 806668
00:02:29.520 28   swift-nioPackageTests.xctest       0x00000000004c063f <unavailable> + 788031
00:02:29.520 29   swift-nioPackageTests.xctest       0x000000000056bb4a <unavailable> + 1489738
00:02:29.521 30   swift-nioPackageTests.xctest       0x000000000056aaa1 <unavailable> + 1485473
00:02:29.521 31   swift-nioPackageTests.xctest       0x000000000056ab49 <unavailable> + 1485641
00:02:29.521 32   libpthread.so.0                    0x00007f8170258184 <unavailable> + 33156
00:02:29.522 33   libc.so.6                          0x00007f816e82dfd0 clone + 109

@swift-nio-bot test this please

Lukasa added some commits Apr 6, 2018

@Lukasa

This comment has been minimized.

Copy link
Contributor Author

Lukasa commented Apr 6, 2018

@normanmaurer Please re-review.

@normanmaurer normanmaurer merged commit 5b2ad2d into apple:master Apr 7, 2018

2 checks passed

pull request validation (4.0.3) Build finished.
Details
pull request validation (4.1) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 7, 2018

@Lukasa thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment