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

netty-codec: Manage read-flow explicitly in MessageAggregator #6604

Closed
wants to merge 1 commit into from
Closed

netty-codec: Manage read-flow explicitly in MessageAggregator #6604

wants to merge 1 commit into from

Conversation

vkostyukov
Copy link
Contributor

@vkostyukov vkostyukov commented Apr 5, 2017 •
edited
Loading

Motivation:

MessageAggregator doesn't manage the read-flow when auto-read is disabled, which may result in connection stalls.

Modification:

Keep reading the channel if auto-read is disabled and the current message isn't yet fully aggregated.

Result:

Fixes #6583.

@vkostyukov vkostyukov force-pushed the vk/read-if-needed-message-message-aggregator branch from 2308374 to 75ac8df Compare April 9, 2017 23:22
@vkostyukov
Copy link
Contributor Author

Okay, I added a new MessageAggregatorTest that mocks a message aggregator instance and only test the read-flow management for now. Please, let me know what do you think about that.

@vkostyukov vkostyukov force-pushed the vk/read-if-needed-message-message-aggregator branch from 75ac8df to 8a0e852 Compare April 9, 2017 23:54
@vkostyukov
Copy link
Contributor Author

Ping @Scottmitch, @normanmaurer. Please, let me know if there is anything you think could be improved here.

normanmaurer
normanmaurer requested changes Apr 15, 2017
View reviewed changes
codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated

protected MockMessageAggregator() {
super(1024);
}
Copy link
Member

Choose a reason for hiding this comment

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

please fix formatting

codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated
ReadCounter counter = new ReadCounter();
ByteBufHolder first = new DefaultByteBufHolder(Unpooled.wrappedBuffer("first".getBytes()));
ByteBufHolder chunk = new DefaultByteBufHolder(Unpooled.wrappedBuffer("chunk".getBytes()));
ByteBufHolder last = new DefaultByteBufHolder(Unpooled.wrappedBuffer("last".getBytes()));
Copy link
Member

Choose a reason for hiding this comment

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

use Unpooled.copiedBuffer(..., CharsetUtil.US_ASCII);

codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated
assertTrue(embedded.writeInbound(last));

assertEquals(3, counter.value); // 2 reads issued from MockMessageAggregator
// 1 read issued from EmbeddedChannel constructor
Copy link
Member

Choose a reason for hiding this comment

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

fix formatting

codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated
assertEquals(3, counter.value); // 2 reads issued from MockMessageAggregator
// 1 read issued from EmbeddedChannel constructor
assertTrue(embedded.finish());
}
Copy link
Member

Choose a reason for hiding this comment

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

use embedded.readInbound() and so train the read messages. Also assert its what you expect and ensure you release everything

codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated
import io.netty.channel.embedded.EmbeddedChannel;

public class MessageAggregatorTest {
static class ReadCounter extends ChannelOutboundHandlerAdapter {
Copy link
Member

Choose a reason for hiding this comment

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

private final

@vkostyukov vkostyukov force-pushed the vk/read-if-needed-message-message-aggregator branch 2 times, most recently from b908d09 to ba22048 Compare April 15, 2017 17:07
@vkostyukov
Copy link
Contributor Author

@normanmaurer thank a lot of the review The PR is updated!

@vkostyukov vkostyukov force-pushed the vk/read-if-needed-message-message-aggregator branch from ba22048 to d2d82e2 Compare April 15, 2017 17:15
normanmaurer
normanmaurer requested changes Apr 16, 2017
View reviewed changes
codec/src/test/java/io/netty/handler/codec/MessageAggregatorTest.java Outdated

private static ByteBufHolder message(String string) {
return new DefaultByteBufHolder(
Unpooled.copiedBuffer(string.getBytes(CharsetUtil.US_ASCII)));
Copy link
Member

Choose a reason for hiding this comment

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

nit: Unpooled.copiedBuffer(string, CharsetUtil.US_ASCII);

No need to first convert to byte[] and then do the copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Just updated the PR.

@vkostyukov vkostyukov force-pushed the vk/read-if-needed-message-message-aggregator branch from d2d82e2 to cc8cd3e Compare April 16, 2017 18:25
@normanmaurer normanmaurer self-assigned this Apr 17, 2017
@normanmaurer
Copy link
Member

Cherry-picked as 4c77e7c into 4.1

@normanmaurer normanmaurer mentioned this pull request Apr 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanmaurer normanmaurer normanmaurer requested changes

Assignees

@normanmaurer normanmaurer

Labels
defect
Projects
None yet
Milestone
4.1.10.Final
Development

Successfully merging this pull request may close these issues.
2 participants
@vkostyukov @normanmaurer