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

Close Channel on Error #183

Merged
merged 3 commits into from Feb 26, 2019
Merged

Close Channel on Error #183

merged 3 commits into from Feb 26, 2019

Conversation

stephencelis
Copy link
Member

Fixes #182.

We probably want to add some kind of logging, too.

@@ -107,6 +107,10 @@ private final class Handler: ChannelInboundHandler {
}
}
}

func errorCaught(ctx: ChannelHandlerContext, error: Error) {
_ = ctx.close()
Copy link

Choose a reason for hiding this comment

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

if you use ctx.close(promise: nil) you can save one allocation for when you don't care about the result anyway (I realise my example code didn't do that ;) )

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know, thanks! We've only started to dig deeper into NIO from our current lightweight wrapper. We've got a branch going that redefines middleware as (Conn<I, A>) -> EventLoopFuture<Conn<J, B>> everywhere instead of our current stack built on IO: https://github.com/pointfreeco/swift-web/compare/nio-conn

Haven't been able to fully flesh it out yet, though!

Copy link

Choose a reason for hiding this comment

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

wow, nice. That sounds awesome!

headers: .init([("location", self.baseUrl.absoluteString)])
)
),
promise: nil
Copy link
Member Author

Choose a reason for hiding this comment

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

@weissi Is it safe to assume that we should favor these promise: nil overloads over _ = wherever we're not then-chaining?

Copy link

Choose a reason for hiding this comment

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

yes, there's no difference apart from allocating the promise/future that you'd then throw away with _ = . Therefore if you don't need the result, just passing promise: nil is just a bit cheaper. Here's the NIO code:

https://github.com/apple/swift-nio/blob/6050c9c864d641f66114cfd6ab690f1aa51d74d4/Sources/NIO/ChannelInvoker.swift#L141-L144

Copy link
Member

@mbrandonw mbrandonw left a comment

Choose a reason for hiding this comment

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

awesome, good to know!

@stephencelis stephencelis merged commit a968110 into master Feb 26, 2019
@stephencelis stephencelis deleted the close-on-error branch February 26, 2019 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants