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 new protocol for ChannelHandler to get buffered bytes in the channel handler #2918

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

Conversation

johnnzhou
Copy link
Contributor

@johnnzhou johnnzhou commented Oct 13, 2024

Add new protocol to get buffered bytes from ChannelHandlers and ChannelPipeline API to query the buffered bytes from ChannelHandlers

Motivation:

In #2849, a new ChannelOption is introduced to retrieve the number of buffered outbound bytes in a Channel. However, this solution does not account for bytes that may be buffered within individual ChannelHandlers. This PR builds on #2849 by adding functionality to audit buffered bytes residing in ChannelHandlers and exposing this information through new ChannelPipeline APIs.

Modifications:

  • Two new protocols for ChannelHandler to audit buffered bytes for inbound and outbound.
    • NIOOutboundByteBufferingChannelHandler
    • NIOInboundByteBufferingChannelHandler
  • New ChannelPipeline APIs
    • outboundBufferedBytes()
    • outboundBufferedBytes(in: ChannelHandlerContext)
    • inboundBufferedBytes()
    • inboundBufferedBytes(in: ChannelHandlerContext)

Result:

Users can now easily query the amount of bytes buffered in ChannelHandlers using the new ChannelPipeline APIs, enhancing the visibility of ChannelHandler performance.

@Lukasa Lukasa added the semver/minor Adds new public API. label Oct 14, 2024
@Lukasa
Copy link
Contributor

Lukasa commented Oct 14, 2024

Thanks so much for opening this @johnnzhou! cc @weissi @Joannis @FranzBusch who are all going to care about this API.

Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

In general I am happy to see an API added for this. I am a bit unsure if the word audit is the right one here. Could we just drop that from all the APIs here? What do others think?

@johnnzhou
Copy link
Contributor Author

I am a bit unsure if the word audit is the right one here. Could we just drop that from all the APIs here? What do others think?

Hi @FranzBusch, thanks for this suggestion. I originally designed this functionality as a way for users to inspect the number of bytes buffered in the channel handlers. Perhaps audit is more suited for financial contexts.

How about we use the word count instead? For instance, we could rename the protocol to NIOOutboundBufferedBytesCountableChannelHandler and the public method to countOutboundBufferedBytes. This change would preserve the idea of inspection while emphasizing the action of counting, making it more intuitive.

What do you think?

@Joannis
Copy link

Joannis commented Oct 16, 2024

Would this PR also (intend to) support child channels, such as in HTTP/2.

@FranzBusch
Copy link
Member

How about we use the word count instead? For instance, we could rename the protocol to NIOOutboundBufferedBytesCountableChannelHandler and the public method to countOutboundBufferedBytes. This change would preserve the idea of inspection while emphasizing the action of counting, making it more intuitive.

Do we need count even here? What about NIOOutboundByteBufferingChannelHandler? The current name of the property sounds good to me.

Would this PR also (intend to) support child channels, such as in HTTP/2.

Depends what you expect. Every ChannelHandler that does multiplexing such as the H2Handler needs to decide how to treat. When a handler in a stream channel asks about the outbound buffered bytes it could return both the buffer from just the stream or the whole connection. We need to decide and implement that on a case-by-case basis.

@johnnzhou
Copy link
Contributor Author

Do we need count even here? What about NIOOutboundByteBufferingChannelHandler? The current name of the property sounds good to me.

I'm generally ok with NIOOutboundByteBufferingChannelHandler. However, my main concern is that this name might be a bit misleading. Two new protocols are not intended to help provide buffering mechanism or manage bytes storage in the channel handlers. Instead, their main function is to collect the number of bytes buffered in the channel handler and allow users to make informed decision on their network load.

Would this PR also (intend to) support child channels, such as in HTTP/2.

Depends what you expect. Every ChannelHandler that does multiplexing such as the H2Handler needs to decide how to treat. When a handler in a stream channel asks about the outbound buffered bytes it could return both the buffer from just the stream or the whole connection. We need to decide and implement that on a case-by-case basis.

My original plan is to implement this protocol for channel handlers provided by NIO. But maybe we need to make separate PRs for each handler?

@FranzBusch
Copy link
Member

I'm generally ok with NIOOutboundByteBufferingChannelHandler. However, my main concern is that this name might be a bit misleading. Two new protocols are not intended to help provide buffering mechanism or manage bytes storage in the channel handlers. Instead, their main function is to collect the number of bytes buffered in the channel handler and allow users to make informed decision on their network load.

I understand where you are coming from. I personally still prefer that name but I would like to hear what @Lukasa @glbrntt or @weissi think.

My original plan is to implement this protocol for channel handlers provided by NIO. But maybe we need to make separate PRs for each handler?

We should definitely separate the API additions in this PR from the adoption.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 17, 2024

I'm happy for us to go with NIOOutboundByteBufferingChannelHandler

@johnnzhou
Copy link
Contributor Author

johnnzhou commented Oct 17, 2024

Sounds good. I will make the change accordingly. I will add more test cases in other commits.

Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Getting closer. Thanks for the work. I left some comments inline

Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ChannelPipeline.swift Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants