Motivation: (#10326)

The current FLowControlHandler keeps a flag to track whether a read() call is pending.
This could lead to a scenario where you call read multiple times when the queue is empty,
and when the FlowControlHandler Queue starts getting messages, channelRead will be fired only once,
when we should've fired x many times, once for each time the handlers downstream called read().

Modifications:

Minor change to replace the boolean flag with a counter and adding a unit test for this scenario.

Result:

I used TDD, so I wrote the test, made sure it's failing, then updated the code and re-ran the test
to make sure it's successful after the changes.

Co-authored-by: Kareem Ali <kali@localhost.localdomain>
This commit is contained in:
Kareem Ali 2020-06-04 10:17:33 -07:00 committed by GitHub
parent 714dd00aab
commit b559711f3e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 68 additions and 6 deletions

View File

@ -74,12 +74,17 @@ public class FlowControlHandler extends ChannelDuplexHandler {
private ChannelConfig config; private ChannelConfig config;
private boolean shouldConsume; private int readRequestCount;
public FlowControlHandler() { public FlowControlHandler() {
this(true); this(true);
} }
/**
* @param releaseMessages If {@code false}, the handler won't release the buffered messages
* when the handler is removed.
*
*/
public FlowControlHandler(boolean releaseMessages) { public FlowControlHandler(boolean releaseMessages) {
this.releaseMessages = releaseMessages; this.releaseMessages = releaseMessages;
} }
@ -140,7 +145,7 @@ public class FlowControlHandler extends ChannelDuplexHandler {
// It seems no messages were consumed. We need to read() some // It seems no messages were consumed. We need to read() some
// messages from upstream and once one arrives it need to be // messages from upstream and once one arrives it need to be
// relayed to downstream to keep the flow going. // relayed to downstream to keep the flow going.
shouldConsume = true; ++readRequestCount;
ctx.read(); ctx.read();
} }
} }
@ -156,8 +161,8 @@ public class FlowControlHandler extends ChannelDuplexHandler {
// We just received one message. Do we need to relay it regardless // We just received one message. Do we need to relay it regardless
// of the auto reading configuration? The answer is yes if this // of the auto reading configuration? The answer is yes if this
// method was called as a result of a prior read() call. // method was called as a result of a prior read() call.
int minConsume = shouldConsume ? 1 : 0; int minConsume = Math.min(readRequestCount, queue.size());
shouldConsume = false; readRequestCount -= minConsume;
dequeue(ctx, minConsume); dequeue(ctx, minConsume);
} }

View File

@ -304,7 +304,7 @@ public class FlowControlHandlerTest {
// channelRead(2) // channelRead(2)
peer.config().setAutoRead(true); peer.config().setAutoRead(true);
setAutoReadLatch1.countDown(); setAutoReadLatch1.countDown();
assertTrue(msgRcvLatch1.await(1L, SECONDS)); assertTrue(msgRcvLatch2.await(1L, SECONDS));
// channelRead(3) // channelRead(3)
peer.config().setAutoRead(true); peer.config().setAutoRead(true);
@ -353,7 +353,7 @@ public class FlowControlHandlerTest {
// Write the message // Write the message
client.writeAndFlush(newOneMessage()) client.writeAndFlush(newOneMessage())
.syncUninterruptibly(); .syncUninterruptibly();
// channelRead(1) // channelRead(1)
peer.read(); peer.read();
@ -373,6 +373,63 @@ public class FlowControlHandlerTest {
} }
} }
/**
* The {@link FlowControlHandler} will keep track of read calls when
* when read is called multiple times when the FlowControlHandler queue is empty.
*/
@Test
public void testTrackReadCallCount() throws Exception {
final Exchanger<Channel> peerRef = new Exchanger<Channel>();
final CountDownLatch msgRcvLatch1 = new CountDownLatch(1);
final CountDownLatch msgRcvLatch2 = new CountDownLatch(2);
final CountDownLatch msgRcvLatch3 = new CountDownLatch(3);
ChannelInboundHandlerAdapter handler = new ChannelDuplexHandler() {
@Override
public void channelActive(ChannelHandlerContext ctx) throws Exception {
ctx.fireChannelActive();
peerRef.exchange(ctx.channel(), 1L, SECONDS);
}
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) {
msgRcvLatch1.countDown();
msgRcvLatch2.countDown();
msgRcvLatch3.countDown();
}
};
FlowControlHandler flow = new FlowControlHandler();
Channel server = newServer(false, flow, handler);
Channel client = newClient(server.localAddress());
try {
// The client connection on the server side
Channel peer = peerRef.exchange(null, 1L, SECONDS);
// Confirm that the queue is empty
assertTrue(flow.isQueueEmpty());
// Request read 3 times
peer.read();
peer.read();
peer.read();
// Write the message
client.writeAndFlush(newOneMessage())
.syncUninterruptibly();
// channelRead(1)
assertTrue(msgRcvLatch1.await(1L, SECONDS));
// channelRead(2)
assertTrue(msgRcvLatch2.await(1L, SECONDS));
// channelRead(3)
assertTrue(msgRcvLatch3.await(1L, SECONDS));
assertTrue(flow.isQueueEmpty());
} finally {
client.close();
server.close();
}
}
@Test @Test
public void testReentranceNotCausesNPE() throws Throwable { public void testReentranceNotCausesNPE() throws Throwable {
final Exchanger<Channel> peerRef = new Exchanger<Channel>(); final Exchanger<Channel> peerRef = new Exchanger<Channel>();