Fix Channelz window reporting

Both Netty and OkHttp had local and remote windows flipped. Also, the
comment in OkHttp about "not tracked" wasn't true, so make it more
accurate and provide a _hint_ of what the window may be instead of just
-1.
This commit is contained in:
Eric Anderson 2022-06-27 12:29:57 -07:00 committed by GitHub
parent a07304471b
commit fad38f49f1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 25 additions and 23 deletions

View File

@ -466,8 +466,11 @@ class Utils {
private final Http2FlowController remote; private final Http2FlowController remote;
FlowControlReader(Http2Connection connection) { FlowControlReader(Http2Connection connection) {
local = connection.local().flowController(); // 'local' in Netty is the _controller_ that controls inbound data. 'local' in Channelz is
remote = connection.remote().flowController(); // the _present window_ provided by the remote that allows data to be sent. They are
// opposites.
local = connection.remote().flowController();
remote = connection.local().flowController();
connectionStream = connection.connectionStream(); connectionStream = connection.connectionStream();
} }

View File

@ -456,8 +456,8 @@ public abstract class NettyHandlerTestBase<T extends Http2ConnectionHandler> {
public void transportTracer_windowSizeDefault() throws Exception { public void transportTracer_windowSizeDefault() throws Exception {
manualSetUp(); manualSetUp();
TransportStats transportStats = transportTracer.getStats(); TransportStats transportStats = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, transportStats.remoteFlowControlWindow); assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, transportStats.localFlowControlWindow);
assertEquals(flowControlWindow, transportStats.localFlowControlWindow); assertEquals(flowControlWindow, transportStats.remoteFlowControlWindow);
} }
@Test @Test
@ -465,31 +465,31 @@ public abstract class NettyHandlerTestBase<T extends Http2ConnectionHandler> {
flowControlWindow = 1024 * 1024; flowControlWindow = 1024 * 1024;
manualSetUp(); manualSetUp();
TransportStats transportStats = transportTracer.getStats(); TransportStats transportStats = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, transportStats.remoteFlowControlWindow); assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, transportStats.localFlowControlWindow);
assertEquals(flowControlWindow, transportStats.localFlowControlWindow); assertEquals(flowControlWindow, transportStats.remoteFlowControlWindow);
} }
@Test @Test
public void transportTracer_windowUpdate_remote() throws Exception { public void transportTracer_windowUpdate_remote() throws Exception {
manualSetUp(); manualSetUp();
TransportStats before = transportTracer.getStats(); TransportStats before = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.remoteFlowControlWindow);
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.localFlowControlWindow); assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.localFlowControlWindow);
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.remoteFlowControlWindow);
ByteBuf serializedSettings = windowUpdate(0, 1000); ByteBuf serializedSettings = windowUpdate(0, 1000);
channelRead(serializedSettings); channelRead(serializedSettings);
TransportStats after = transportTracer.getStats(); TransportStats after = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE + 1000, assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE + 1000,
after.remoteFlowControlWindow); after.localFlowControlWindow);
assertEquals(flowControlWindow, after.localFlowControlWindow); assertEquals(flowControlWindow, after.remoteFlowControlWindow);
} }
@Test @Test
public void transportTracer_windowUpdate_local() throws Exception { public void transportTracer_windowUpdate_local() throws Exception {
manualSetUp(); manualSetUp();
TransportStats before = transportTracer.getStats(); TransportStats before = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.remoteFlowControlWindow); assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, before.localFlowControlWindow);
assertEquals(flowControlWindow, before.localFlowControlWindow); assertEquals(flowControlWindow, before.remoteFlowControlWindow);
// If the window size is below a certain threshold, netty will wait to apply the update. // If the window size is below a certain threshold, netty will wait to apply the update.
// Use a large increment to be sure that it exceeds the threshold. // Use a large increment to be sure that it exceeds the threshold.
@ -497,7 +497,7 @@ public abstract class NettyHandlerTestBase<T extends Http2ConnectionHandler> {
connection().connectionStream(), 8 * Http2CodecUtil.DEFAULT_WINDOW_SIZE); connection().connectionStream(), 8 * Http2CodecUtil.DEFAULT_WINDOW_SIZE);
TransportStats after = transportTracer.getStats(); TransportStats after = transportTracer.getStats();
assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, after.remoteFlowControlWindow); assertEquals(Http2CodecUtil.DEFAULT_WINDOW_SIZE, after.localFlowControlWindow);
assertEquals(flowControlWindow + 8 * Http2CodecUtil.DEFAULT_WINDOW_SIZE, assertEquals(flowControlWindow + 8 * Http2CodecUtil.DEFAULT_WINDOW_SIZE,
connection().local().flowController().windowSize(connection().connectionStream())); connection().local().flowController().windowSize(connection().connectionStream()));
} }

View File

@ -323,8 +323,10 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
@Override @Override
public TransportTracer.FlowControlWindows read() { public TransportTracer.FlowControlWindows read() {
synchronized (lock) { synchronized (lock) {
long local = -1; // okhttp does not track the local window size long local = outboundFlow == null ? -1 : outboundFlow.windowUpdate(null, 0);
long remote = outboundFlow == null ? -1 : outboundFlow.windowUpdate(null, 0); // connectionUnacknowledgedBytesRead is only readable by ClientFrameHandler, so we
// provide a lower bound.
long remote = (long) (initialWindowSize * DEFAULT_WINDOW_UPDATE_RATIO);
return new TransportTracer.FlowControlWindows(local, remote); return new TransportTracer.FlowControlWindows(local, remote);
} }
} }

View File

@ -727,24 +727,21 @@ public class OkHttpClientTransportTest {
public void transportTracer_windowSizeDefault() throws Exception { public void transportTracer_windowSizeDefault() throws Exception {
initTransport(); initTransport();
TransportStats stats = getTransportStats(clientTransport); TransportStats stats = getTransportStats(clientTransport);
assertEquals(INITIAL_WINDOW_SIZE, stats.remoteFlowControlWindow); assertEquals(INITIAL_WINDOW_SIZE / 2, stats.remoteFlowControlWindow); // Lower bound
// okhttp does not track local window sizes assertEquals(INITIAL_WINDOW_SIZE, stats.localFlowControlWindow);
assertEquals(-1, stats.localFlowControlWindow);
} }
@Test @Test
public void transportTracer_windowSize_remote() throws Exception { public void transportTracer_windowSize_remote() throws Exception {
initTransport(); initTransport();
TransportStats before = getTransportStats(clientTransport); TransportStats before = getTransportStats(clientTransport);
assertEquals(INITIAL_WINDOW_SIZE, before.remoteFlowControlWindow); assertEquals(INITIAL_WINDOW_SIZE / 2, before.remoteFlowControlWindow); // Lower bound
// okhttp does not track local window sizes assertEquals(INITIAL_WINDOW_SIZE, before.localFlowControlWindow);
assertEquals(-1, before.localFlowControlWindow);
frameHandler().windowUpdate(0, 1000); frameHandler().windowUpdate(0, 1000);
TransportStats after = getTransportStats(clientTransport); TransportStats after = getTransportStats(clientTransport);
assertEquals(INITIAL_WINDOW_SIZE + 1000, after.remoteFlowControlWindow); assertEquals(INITIAL_WINDOW_SIZE / 2, after.remoteFlowControlWindow);
// okhttp does not track local window sizes assertEquals(INITIAL_WINDOW_SIZE + 1000, after.localFlowControlWindow);
assertEquals(-1, after.localFlowControlWindow);
} }
@Test @Test