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

MicroExpress should close the Channel if an error is received in the last ChannelHandler #8

Closed
weissi opened this issue Feb 25, 2019 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@weissi
Copy link

weissi commented Feb 25, 2019

In SwiftNIO it's generally a good idea that on errorCaught, the Channel should be closed to prevent leaking network connections that are in some error states. In MicroExpress this should be added to HTTPHandler. But this advice applies to any NIO ChannelPipeline: Generally the last handler (I usually call it the 'application handler') should close on unknown error.

@helje5 I'll only file this bug here, just picked one of your projects but there might be others that also don't actively close on errorCaught.

Why? Generally, ChannelHandlers fire errors in cases where they don't know how to recover themselves. If another, later ChannelHandler knows how to recover, that ChannelHandler would consume the error. In other words: If an error reaches the end of the pipeline, nobody knew how to handle that error and closing the Channel is a good default.

This has always been true in SwiftNIO but in NIO 2 this will become especially important because ByteToMessageDecoders (and the HTTP decoders) won't close the connections themselves anymore. Why is that? If the decoders themselves close the connection, then there's no way for the user to opt out.

The code to implement this is very straightforward:

public func errorCaught(ctx: ChannelHandlerContext, error: Error) {
    switch error {
    case let e as SomeErrorTypeIKnowHowToHandle where e.someCondition == expectedValue:
        // handle error
    case let e as SomeOtherErrorIKnowHowToHandle:
        // handle that error
    default:
        // log error?
        ctx.close(promise: nil)
    }
}
@helje5 helje5 self-assigned this Feb 26, 2019
@helje5 helje5 added the bug Something isn't working label Feb 26, 2019
helje5 added a commit that referenced this issue Mar 15, 2019
Implement errorCaught as suggested by @weissi in issue #8.
@helje5
Copy link
Member

helje5 commented Mar 15, 2019

Done.

@helje5 helje5 closed this as completed Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants