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

NettyDataBufferFactory.join should return original buffer as-is in case of a single element (for compatibility with Netty 4.1.32) [SPR-17560] #22092

Closed
spring-issuemaster opened this issue Dec 3, 2018 · 5 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Juergen Hoeller opened SPR-17560 and commented

NettyDataBufferFactory.join(List<DataBuffer>) always creates a CompositeByteBuf, even for a single-element list of original buffers. This is generally worth optimizing but particularly necessary for Netty 4.1.32 where CompositeByteBuf is enforced to have at least two elements (see our current HttpServerTests failures on CI).


Affects: 5.0.11, 5.1.3

Referenced from: commits d5dab12, c5428e6, abf9ce8

Backported to: 5.0.12

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 3, 2018

Juergen Hoeller commented

Rossen Stoyanchev, Arjen Poutsma, I hope this is ok the way I addressed it. As indicated by the recent HttpServerTests failures, we can run into situations with just a single element to join there, and Netty 4.1.32 has an assertion for at least two elements on any CompositeByteBuf.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Oleg Alexeyev commented

The fix is correct, but not complete IMO. I would raise a question why maxNumComponents passed to CompositeByteBuf is equal to the number of buffers. The way maxNumComponents is used affects how buffers are consolidated. Some CompositeByteBuf methods' performance depend on how many buffers inside, but most do not. In particular, parsing the content sequentially doesn't really depend on that and so consolidation is rarely needed at all, meaning that maxNumComponents = Integer.MAX_VALUE seems to be quite safe default - unnecessary consolidation only adds more CPU load for copying data.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Rossen Stoyanchev commented

Oleg Alexeyev as far as I can see CompositeByteBuf will take the lesser of the given maxNumComponents or AbstractByteBufAllocator.DEFAULT_MAX_COMPONENTS which is 16. So wouldn't a value of Integer.MAX_VALUE get ignored?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 5, 2018

Arjen Poutsma commented

Juergen Hoeller Looks good to me. 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 6, 2018

Oleg Alexeyev commented

Rossen Stoyanchev, I'm looking at netty 4.1.31 sources - this is what newList does for creating a ComponentList, consolidateIfNeeded() uses what's assigned to maxNumComponents, and that value is assigned as is. So, no, Integer.MAX_VALUE won't be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.