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

B2MH nil force unwrap #902

Closed
tanner0101 opened this Issue Mar 13, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@tanner0101
Copy link
Contributor

tanner0101 commented Mar 13, 2019

Expected behavior

ByteToMessageHandler can be removed from the pipeline and re-added later.

Actual behavior

Re-adding a B2MH causes a nil force unwrap here: https://github.com/apple/swift-nio/blob/master/Sources/NIO/Codec.swift#L467

Problem code

// re-add HTTPDecoder since it may consider the connection to be closed
_ = context.pipeline.removeHandler(httpResDecoder)
_ = context.pipeline.addHandler(httpResDecoder, position: .after(httpReqEncoder))

Fixed code

// re-add HTTPDecoder since it may consider the connection to be closed
_ = context.pipeline.removeHandler(httpResDecoder)
let newResDecoder = ByteToMessageHandler(HTTPResponseDecoder())
_ = context.pipeline.addHandler(newResDecoder, position: .after(httpReqEncoder))

Note

I'm not sure if I actually need to be re-adding the HTTPDecoder anymore here. I had to do this previously to reset the handler since it was viewing the connection as closed after a successful proxy request.

SwiftNIO version/commit hash

2.0.0-convergence.1

Swift & OS version (output of swift --version && uname -a)

Apple Swift version 5.0 (swiftlang-1001.0.69 clang-1001.0.45.2)
Target: x86_64-apple-darwin18.2.0
Darwin iMac.home 18.2.0 Darwin Kernel Version 18.2.0: Mon Nov 12 20:24:46 PST 2018; root:xnu-4903.231.4~2/RELEASE_X86_64 x86_64
@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 13, 2019

@tanner0101 most handlers do really not expect that the same instance is to be re-added to a pipeline. We could support this in B2MH but the problem is that random B2MD implementation really won't expect that I think.

I'm probably +0.0001 on making B2MHs re-addable. What does @Lukasa think?

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Mar 13, 2019

I’m inclined to not support this for now. I think it’ll surprise B2MD implementers, and doing it wrongly will lead to hilariously difficult to diagnose bugs. To reset a handler, I think it’s sufficient to just add a new one.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 13, 2019

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Mar 13, 2019

@weissi @Lukasa makes sense to me. But maybe we could have an assertion / fatalError here that has a more helpful debug message? It wasn't immediately obvious to me that this wasn't allowed or what the problem was.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 13, 2019

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Mar 13, 2019

I think the best we can do is an assertion or fatalError. handlerAdded doesn't throw or accept a promise:

func handlerAdded(context: ChannelHandlerContext)
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 13, 2019

weissi added a commit to weissi/swift-nio that referenced this issue Mar 14, 2019

B2MH: helpful message if re-added to pipeline
Motivation:

B2MH used to crash a nil variable if re-added to a pipeline.

Modifications:

provide good error message for the crash instead

Result:

- fixes apple#902

@weissi weissi added the fix-proposed label Mar 14, 2019

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Mar 14, 2019

better crash message for now ^^

@weissi weissi closed this in #903 Mar 14, 2019

weissi added a commit that referenced this issue Mar 14, 2019

B2MH: helpful message if re-added to pipeline (#903)
Motivation:

B2MH used to crash a nil variable if re-added to a pipeline.

Modifications:

provide good error message for the crash instead

Result:

- fixes #902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.