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.

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

Issue-2734 - Clamp buffer to maximum upon large write operation #2745

Merged
merged 18 commits into from
Sep 18, 2024
Merged

Issue-2734 - Clamp buffer to maximum upon large write operation #2745

merged 18 commits into from
Sep 18, 2024

Conversation

ali-ahsan-ali
Copy link
Contributor

Fix Issue 2734 by clamping storage size after large write operation

Motivation:

Fix Issue 2734

Modifications:

  • Added a function to clamp storage by copying bytes and setting new capacity of storage
  • Adding a function to clamp the capacity of ByteBuffer
  • Added the ability to specify he maxBufferCapacity to MessageToByteHandler

Result:

Once a write message that is larger than the capacity of the MessageToByteHandler's maxBufferCapacity, it clamps the byteBuffer down.

@ali-ahsan-ali
Copy link
Contributor Author

Addressing #2734

@Lukasa Lukasa added the semver/patch No public API change. label Jun 20, 2024
Lukasa
Lukasa requested changes Jun 20, 2024
View reviewed changes
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.

Thanks for this! I've left a few notes in the diff.

Tests/NIOCoreTests/ByteBufferTest.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
FranzBusch
FranzBusch reviewed Jun 26, 2024
View reviewed changes
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2734-shrink-buffer-after-write branch from ebb1d9c to 825a6b3 Compare July 4, 2024 08:19
FranzBusch
FranzBusch reviewed Jul 10, 2024
View reviewed changes
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.

Except the doc nit this look good to me now. I would like @Lukasa to also take a look before merging.

Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
Lukasa
Lukasa reviewed Jul 22, 2024
View reviewed changes
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2734-shrink-buffer-after-write branch from c64d301 to 180b41f Compare July 27, 2024 10:18
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2734-shrink-buffer-after-write branch from 180b41f to a48b322 Compare July 27, 2024 10:18
@weissi weissi requested a review from Lukasa August 5, 2024 09:14
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2734-shrink-buffer-after-write branch from 3c4c678 to 3da1d3a Compare August 6, 2024 10:09
Lukasa
Lukasa approved these changes Aug 9, 2024
View reviewed changes
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, I'm happy with this if @weissi is.

weissi
weissi requested changes Aug 9, 2024
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the patch. I think I found a UB issue and I'm not sure if the reader/writerIndices are handled correctly either

Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/Codec.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali
Copy link
Contributor Author

Hey @weissi LMK when you find time to take a look at this. No rush!

weissi
weissi reviewed Sep 10, 2024
View reviewed changes
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved
@weissi
Copy link
Member

weissi commented Sep 10, 2024

Hey @weissi LMK when you find time to take a look at this. No rush!

Commented, looks 99% complete, just three small things

weissi
weissi approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@weissi weissi enabled auto-merge (squash) September 13, 2024 13:41
@ali-ahsan-ali
Copy link
Contributor Author

Will cry tears of joy once this gets merged 👍

Lukasa
Lukasa approved these changes Sep 16, 2024
View reviewed changes
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.

Very nice, thank you very much for your hard work and persistence with this!

auto-merge was automatically disabled September 17, 2024 02:03

Head branch was pushed to by a user without write access

@Lukasa Lukasa enabled auto-merge (squash) September 17, 2024 12:58
@Lukasa Lukasa merged commit 8307ad6 into apple:main Sep 18, 2024
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FranzBusch FranzBusch FranzBusch left review comments

@weissi weissi weissi approved these changes

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ali-ahsan-ali @weissi @Lukasa @FranzBusch