Skip to content

Conform RedisByteDecoder to NIOSingleStepByteToMessageDecoder #63

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
Merged
fabianfett merged 3 commits into from
Jun 19, 2023
Merged

Conform RedisByteDecoder to NIOSingleStepByteToMessageDecoder #63

fabianfett merged 3 commits into from
Jun 19, 2023

Conversation

@fabianfett
Copy link
Member

@fabianfett fabianfett commented Jun 18, 2023

Cherry pick of:
https://gitlab.com/swift-server-community/RediStack/-/merge_requests/185

Motivation

Traffic on the NIOChannelPipeline is not free, because of this recent NIO projects try to reduce the number of needed handlers for a given protocol. For example NIOSSH and NIOHTTP2 parse incoming bytes in the main ChannelHandler rather than within a dedicated DecoderHandler. To make this pattern easier the NIOSingleStepByteToMessageDecoder was introduced into NIO. The NIOSingleStepByteToMessageDecoder allows users to have an explicit decoder object within their main channel handler, that has the same semantics as the ByteToMessageDecoder in the channel pipeline. A usage example can be seen here:

https://github.com/vapor/postgres-nio/blob/main/Sources/PostgresNIO/New/PostgresChannelHandler.swift#L102

To allow us to move the RedisByteDecoder into the RedisChannelHandler, let's conform it to NIOSingleStepByteToMessageDecoder.

Changes

Conform RedisByteDecoder to NIOSingleStepByteToMessageDecoder. This is a non breaking change since NIOSingleStepByteToMessageDecoder has default implementations for the ByteToMessageDecoder protocol, which we continue to support.

@fabianfett
Copy link
Member Author

fabianfett commented Jun 18, 2023

API breakage tool flags false positives:

No breaking changes detected in RedisTypes

No breaking changes detected in RediStackTestUtils

11 breaking changes detected in RediStack:
  💔 API breakage: class RedisByteDecoder has been changed to a struct
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has return type change from NIOCore.DecodingState to RediStack.RESPValue?
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has parameter 0 type change from NIOCore.ChannelHandlerContext to NIOCore.ByteBuffer
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has parameter 0 changing from Default to InOut
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has return type change from NIOCore.DecodingState to RediStack.RESPValue?
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 0 type change from NIOCore.ChannelHandlerContext to NIOCore.ByteBuffer
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 0 changing from Default to InOut
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 1 type change from NIOCore.ByteBuffer to Swift.Bool
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 1 changing from InOut to Default
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has been renamed to func decode(buffer:)
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has been renamed to func decodeLast(buffer:seenEOF:)

As stated above:

This is a non breaking change since NIOSingleStepByteToMessageDecoder has default implementations for the ByteToMessageDecoder protocol, which we continue to support.

@fabianfett fabianfett added the 🆕 semver/minor Adds new public API. label Jun 18, 2023
@fabianfett fabianfett assigned Joannis and unassigned Joannis Jun 18, 2023
@fabianfett fabianfett requested a review from Joannis June 18, 2023 13:08
@Joannis
Copy link
Member

Joannis commented Jun 18, 2023

@fabianfett isn't the change from class to struct a breaking one?

@fabianfett fabianfett force-pushed the ff-cherry-RedisByteDecoder branch from ff608c9 to f1dbb64 Compare June 19, 2023 08:30
@fabianfett
Copy link
Member Author

fabianfett commented Jun 19, 2023

@fabianfett isn't the change from class to struct a breaking one?

@Joannis Great catch! Fixed!

@fabianfett
Copy link
Member Author

fabianfett commented Jun 19, 2023

Only expected false positive changes as of now:

10 breaking changes detected in RediStack:
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has return type change from NIOCore.DecodingState to RediStack.RESPValue?
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has parameter 0 type change from NIOCore.ChannelHandlerContext to NIOCore.ByteBuffer
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has parameter 0 changing from Default to InOut
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has return type change from NIOCore.DecodingState to RediStack.RESPValue?
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 0 type change from NIOCore.ChannelHandlerContext to NIOCore.ByteBuffer
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 0 changing from Default to InOut
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 1 type change from NIOCore.ByteBuffer to Swift.Bool
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has parameter 1 changing from InOut to Default
  💔 API breakage: func RedisByteDecoder.decode(context:buffer:) has been renamed to func decode(buffer:)
  💔 API breakage: func RedisByteDecoder.decodeLast(context:buffer:seenEOF:) has been renamed to func decodeLast(buffer:seenEOF:)

@fabianfett fabianfett merged commit 11b4673 into main Jun 19, 2023
@fabianfett fabianfett deleted the ff-cherry-RedisByteDecoder branch June 19, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Joannis Joannis Joannis approved these changes

Assignees

No one assigned

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fabianfett @Joannis