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

Add RemovableChannelHandler conformance to MessageToByteHandler #1993

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AlexisQapa
Copy link

@AlexisQapa AlexisQapa commented Nov 23, 2021

Add RemovableChannelHandler conformance to MessageToByteHandler as a follow up for #1991

Motivation:

RemovableChannelHandler conformance was missing MessageToByteHandler while ByteToMessageHandler did conform to it.

Modifications:

Add RemovableChannelHandler conformance to ByteToMessageHandler
Use the same pattern as for ByteToMessage with hooks for encoders to act on removal.
Added hooks have default implementation.

func encodeLast(data: OutboundIn, out: inout ByteBuffer) throws
mutating func encoderRemoved()
mutating func encoderAdded()

edit: removed context from hooks

Result:

MessageToByteHandler now conform to RemovableChannelHandler.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

11 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@AlexisQapa
Copy link
Author

I must say I'm not sure about my implementation as I'm quite new to the code base. I'll look into tests when I can thought any advice about how to would be appreciated. Should this be a draft PR ?

Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
@AlexisQapa
Copy link
Author

AlexisQapa commented Nov 24, 2021

I could turn the RemovalState to a Bool but I'm unsure if it then needs a precondition in handlerAdded.

https://github.com/AlexisQapa/swift-nio/tree/use-boolean-instead-of-removal-state

@Lukasa
Copy link
Contributor

Lukasa commented Nov 24, 2021

You shouldn't need the precondition in handlerAdded.

@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Jan 4, 2022
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Great, this looks really good! One final note: can you add a few unit tests that test the paths through the removal flow here? That way we won't accidentally regress this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants