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

Jump to bottom

Add a Channel Handler that closes the channel #967

Merged

weissi merged 2 commits into apple:master from Palleas:add-error-channel-handler-841

Apr 30, 2019

Contributor

Palleas commented Apr 11, 2019

Motivation:

Because of #600, many users will start leaking Channels because they
don't close on error.

Modifications:

Add ChannelHandlerError
Add ChannelHandlerErrorTest

Result:

User will have access to an handler to use to automatically close
the channel in case of unhandled errors

Closes #841.

swift-nio-bot commented Apr 11, 2019

Can one of the admins verify this patch?

4 similar comments

swift-nio-bot commented Apr 11, 2019

Can one of the admins verify this patch?

swift-nio-bot commented Apr 11, 2019

Can one of the admins verify this patch?

swift-nio-bot commented Apr 11, 2019

Can one of the admins verify this patch?

swift-nio-bot commented Apr 11, 2019

Can one of the admins verify this patch?

Palleas force-pushed the add-error-channel-handler-841 branch from 641d0d0 to 1882ae7 Compare

April 11, 2019 19:31

Lukasa requested changes

View reviewed changes

Contributor

Lukasa left a comment

Cool, generally this looks good! Do you mind addressing the notes I’ve left in the diff?

Sources/NIO/ChannelHandlerError.swift Outdated

		@@ -0,0 +1,11 @@
		import Foundation

		final public class ChannelHandlerError: ChannelInboundHandler {

Contributor

Lukasa Apr 11, 2019

This should be called CloseOnErrorHandler or something similar.

Contributor

Lukasa Apr 11, 2019

Please also update the file names with the new handler name.

Contributor

Lukasa Apr 11, 2019

Actually @normanmaurer is there prior art in Netty here?

Member

normanmaurer Apr 11, 2019

We do not include such a handler in Netty, so no :)

Member

weissi Apr 11, 2019

we also need to prefix this with NIO. I.e. NIOCloseOnErrorHandler because that's the guarantee we gave in the public API declaration.

The other question is: is this swift-nio or swift-nio-extras?

Contributor

Lukasa Apr 12, 2019

Good question. I could go either way there.

Contributor Author

Palleas Apr 12, 2019

Added the NIO prefix, for the rest just let me know 😀

Sources/NIO/ChannelHandlerError.swift Outdated

		@@ -0,0 +1,11 @@
		import Foundation

Contributor

Lukasa Apr 11, 2019

This import of Foundation is unnecessary and can be removed. Additionally, you’ll need to place a license header in this file, as seen in the others.

Tests/NIOTests/ChannelHandlerErrorTest.swift Outdated

		@@ -0,0 +1,46 @@
		import XCTest

Contributor

Lukasa Apr 11, 2019

This file also needs a license header.

Contributor Author

Palleas commented Apr 11, 2019

@Lukasa All done, thanks for the feedback!

normanmaurer requested changes

View reviewed changes

Sources/NIO/CloseOnErrorHandler.swift Outdated

+              //
+              // This source file is part of the SwiftNIO open source project
+              //
+              // Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors

Member

normanmaurer Apr 11, 2019

2019 ?

Contributor Author

Palleas Apr 11, 2019

Oops, I wondered when I copy/pasted 🙈 Fixed!
Do you want me to squash into one commit since only the first matches the template?

Contributor

Lukasa Apr 12, 2019

Squashing the commits would be great!

Contributor Author

Palleas Apr 12, 2019

Done!

Palleas force-pushed the add-error-channel-handler-841 branch from 000bb52 to b441c8a Compare

April 12, 2019 15:25

weissi reviewed

View reviewed changes

Sources/NIO/NIOCloseOnErrorHandler.swift Outdated

+              //
+              //===----------------------------------------------------------------------===//
+              final public class NIOCloseOnErrorHandler: ChannelInboundHandler {

Member

weissi Apr 12, 2019

sorry, super nit but Jazzy renders this as is, would you mind making it public final (instead of final public)
this needs documentation for all the public bits that don't come from at least ChannelInboundHandler, ie. the class and the init

Contributor Author

Palleas Apr 12, 2019

Done! Let me know if you want me to put more details in the docs.

Palleas force-pushed the add-error-channel-handler-841 branch from b441c8a to 6e05f6f Compare

April 12, 2019 15:42

Contributor Author

Palleas commented Apr 18, 2019

@normanmaurer @Lukasa Fixed! Sorry I should have pinged you!

Lukasa reviewed

View reviewed changes

Sources/NIO/NIOCloseOnErrorHandler.swift Outdated

+                  public init() {}
+                  public func errorCaught(context: ChannelHandlerContext, error: Error) {
+                      context.close(promise: nil)

Contributor

Lukasa Apr 18, 2019

Here's a question: should this forward the error on? @weissi?

Member

weissi Apr 18, 2019

@Lukasa I think I agree, yes, don't think this could do anything harmful. In most cases absolutely nothing will happen (because this will likely be the last handler) but in certain cases it might be better to give the user visibility.

Contributor Author

Palleas Apr 18, 2019

So, forward then close? I’m guessing the order matters here

Contributor Author

Palleas Apr 19, 2019

@Lukasa @weissi tried something in 061f163, is that the right approach?

Member

weissi Apr 23, 2019

yes, that works!

Contributor Author

Palleas Apr 23, 2019

Perfect, squashed into one commit!

Palleas force-pushed the add-error-channel-handler-841 branch 3 times, most recently from 63b4fc2 to 34c82c7 Compare

April 23, 2019 15:19

Contributor Author

Palleas commented Apr 23, 2019

@Lukasa @normanmaurer all squashed and ready for another review 🦸🏻‍♂️

Lukasa approved these changes

View reviewed changes

Contributor

Lukasa left a comment

Great, LGTM.

Lukasa added the 🆕 semver/minor label

Lukasa added this to the 2.1.0 milestone

normanmaurer approved these changes

View reviewed changes

Member

normanmaurer left a comment

LGTM... thanks

Contributor

Lukasa commented Apr 29, 2019

@swift-nio-bot test this please

Member

weissi commented Apr 29, 2019

@Palleas your pull request uses windows newlines (\r\n) instead of UNIX newlines (\n) and hence it doesn't pass the license header check tests. Could you run dos2unix Tests/NIOTests/NIOCloseOnErrorHandlerTest.swift Sources/NIO/NIOCloseOnErrorHandler.swift and add all the changes to your PR?

You might also want to fix your text editor to produce UNIX newlines

Contributor Author

Palleas commented Apr 29, 2019

Uh, how did that happen 🤨 I’ll fix that today!


          Add a Channel Handler that closes the channel

104d1ab

Motivation:
Because of apple#600, many users will start leaking Channels because they
don't close on error.

Modifications:

* Add ChannelHandlerError
* Add ChannelHandlerErrorTest

Result:

User will have access to an handler to use to automatically close
the channel in case of unhandled errors

Palleas force-pushed the add-error-channel-handler-841 branch from 2b2d67c to 104d1ab Compare

April 29, 2019 15:46

Contributor Author

Palleas commented Apr 29, 2019

@Lukasa @weissi all done!

Contributor

Lukasa commented Apr 29, 2019

@swift-nio-bot test this please


          Merge branch 'master' into add-error-channel-handler-841

b20c5be

Contributor

Lukasa commented Apr 29, 2019

@swift-nio-bot test this please

weissi merged commit d093ae0 into apple:master

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

Labels

🆕 semver/minor