-
Notifications
You must be signed in to change notification settings - Fork 77
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
Set the max bytes to be read from a connection #118
base: main
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
Thanks for this patch! @weissi In your original issue you suggested using RecvByteBufferAllocator. I kinda think that would be the better choice here as it'd be more compatible with other NIO channels: what do you think? |
@@ -28,6 +28,8 @@ public struct NIOTSChannelOptions { | |||
|
|||
public static let currentPath = NIOTSChannelOptions.Types.NIOTSCurrentPathOption() | |||
|
|||
public static let maxReceiveBytes = NIOTSChannelOptions.Types.NIOMaxReceiveBytesOption() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa I think a separate option (as suggested in this PR) is actually better than matching the socket channel with a RecvAllocator, wdyt? We can't sensibly control the strategy that NW.fw will use so I'm actually happy with this as is, wdyt?
Oh sorry, I missed this. So yeah, initially I thought a |
@Lukasa If we could design What we could obviously do is to add an optional requirement |
Hmm. We kinda need a new allocator protocol for io_uring anyway, don't we? Or am I misunderstanding that? |
I see, yes, that is a good point! What about this PR then, would we want until we have a new allocator for io_uring (that would support not copying the |
I think the new allocator protocol we need for io_uring is potentially very similar to the thing we want here. Might be good to address both tasks in one go? You could always compose the old protocol from the new one, right, so pre-existing channels can kinda trivially support the new API. I don't think adding a |
Yes, agreed.
Fine by me. I think we should take the action to start designing that relatively soon though.
Ok, sounds good to me. Said that, an (optional) |
Add a
NIOTSChannelOption
to configure the maximum number of bytes to be read from a connection.Motivation:
Currently the max number of bytes is hard coded to 8KB. In some scenarios API consumers may want to change this to better suit their project. See also: #92
Modifications:
NIOTSChannelOptions.Types
to represent the new optionConnectionChannelOptions
to store the default or configured valueResult: