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

optional max payload size for ByteToMessageDecoder #957

Merged
merged 4 commits into from Apr 11, 2019

Conversation

Projects
None yet
3 participants
@tomerd
Copy link
Member

tomerd commented Apr 8, 2019

Motivation:

ByteToMessageDecoder aggregate data in memory as part of their normal operation. the ability to limit how much they aggregate is critical in many real-life applications

Modifications:

  • add optional maximumBufferSize argument to ByteToMessageDecoder initializer
  • test for buffer size when maximumBufferSize is set and throw ByteToMessageDecoderError.payloadTooLarge error

Result:

users can limit how much memory ByteToMessageDecoder takes and handle the exception on their end

@tomerd tomerd requested a review from weissi Apr 8, 2019

@weissi
Copy link
Member

weissi left a comment

thanks, this looks really good! A couple of small things though

Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
Show resolved Hide resolved Sources/NIO/Codec.swift Outdated

@tomerd tomerd force-pushed the tomerd:master branch from 81c9a4a to 9304f92 Apr 9, 2019

@tomerd

This comment has been minimized.

Copy link
Member Author

tomerd commented Apr 9, 2019

@weissi @Lukasa feedback addressed + rebased. still one open question re the timing of the check. i can go either way, tho slightly prefer to do it early

Show resolved Hide resolved Sources/NIO/Codec.swift Outdated
@tomerd

This comment has been minimized.

Copy link
Member Author

tomerd commented Apr 10, 2019

@weissi updated

tomerd added some commits Apr 8, 2019

optional max payload size for ByteToMessageDecoder
Motivation:

ByteToMessageDecoder aggregate data in memory as part of their normal operation. the ability to limit how much they aggregate is critical in many real-life applications

Modifications:

* add optional maximumBufferSize argument to ByteToMessageDecoder initializer
* test for buffer size when maximumBufferSize is set and throw ByteToMessageDecoderError.payloadTooLarge error

Result:

users can limit how much memory ByteToMessageDecoder takes and handle the exception on their end

@tomerd tomerd force-pushed the tomerd:master branch 2 times, most recently from c6b1073 to e8220b6 Apr 10, 2019

@tomerd tomerd force-pushed the tomerd:master branch from e8220b6 to 7de4b79 Apr 10, 2019

@weissi

weissi approved these changes Apr 11, 2019

Copy link
Member

weissi left a comment

Thanks @tomerd . That looks great to me!

@Lukasa Lukasa added this to the 2.1.0 milestone Apr 11, 2019

@Lukasa

Lukasa approved these changes Apr 11, 2019

@Lukasa Lukasa merged commit e32a436 into apple:master Apr 11, 2019

1 check passed

pull request validation (5.0) Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.