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

okhttp: okhttp client and server transport should use padded length for flow control #10422

Merged
merged 7 commits into from
Aug 16, 2023
Merged

okhttp: okhttp client and server transport should use padded length for flow control #10422

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f54bfd
fix okhttp padding length
YifeiZhuang Jul 24, 2023
804d9a2
flow control per stream
YifeiZhuang Aug 8, 2023
a06be73
fix
YifeiZhuang Aug 9, 2023
5746ac3
fix server
YifeiZhuang Aug 9, 2023
7702e16
fix test
YifeiZhuang Aug 16, 2023
247c2db
Merge branch 'master' of https://github.com/grpc/grpc-java into fix-o…
YifeiZhuang Aug 16, 2023
624604d
fix error prone
YifeiZhuang Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,10 @@ public void deframeFailed(Throwable cause) {
@Override
@GuardedBy("lock")
public void bytesRead(int processedBytes) {
processedWindow -= processedBytes;
if (processedWindow <= initialWindowSize * Utils.DEFAULT_WINDOW_UPDATE_RATIO) {
int delta = initialWindowSize - processedWindow;
window += delta;
processedWindow += delta;
window = initialWindowSize;
frameWriter.windowUpdate(id(), delta);
}
}
Expand Down Expand Up @@ -321,11 +320,12 @@ public void transportHeadersReceived(List<Header> headers, boolean endOfStream)
* Must be called with holding the transport lock.
*/
@GuardedBy("lock")
public void transportDataReceived(okio.Buffer frame, boolean endOfStream) {
public void transportDataReceived(okio.Buffer frame, boolean endOfStream, int paddingLen) {
// We only support 16 KiB frames, and the max permitted in HTTP/2 is 16 MiB. This is verified
// in OkHttp's Http2 deframer. In addition, this code is after the data has been read.
int length = (int) frame.size();
window -= length;
processedWindow -= (length + paddingLen);
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
if (window < 0) {
frameWriter.rstStream(id(), ErrorCode.FLOW_CONTROL_ERROR);
transport.finishStream(
Expand Down
2 changes: 1 addition & 1 deletion okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ public void data(boolean inFinished, int streamId, BufferedSource in, int length
synchronized (lock) {
// TODO(b/145386688): This access should be guarded by 'stream.transportState().lock';
// instead found: 'OkHttpClientTransport.this.lock'
stream.transportState().transportDataReceived(buf, inFinished);
stream.transportState().transportDataReceived(buf, inFinished, paddedLength - length);
}
}

Expand Down
4 changes: 2 additions & 2 deletions okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,14 +792,14 @@ public void windowUpdate() throws Exception {
frameHandler().data(false, 3, buffer, messageFrameLength, messageFrameLength);

verify(frameWriter, timeout(TIME_OUT_MS))
.windowUpdate(eq(3), eq((long) 2 * messageFrameLength));
.windowUpdate(eq(3), eq((long) 2 * messageFrameLength + paddingLength));

// Stream 2 receives another message
buffer = createMessageFrame(fakeMessage);
frameHandler().data(false, 5, buffer, messageFrameLength, messageFrameLength);

verify(frameWriter, timeout(TIME_OUT_MS))
.windowUpdate(eq(5), eq((long) 2 * messageFrameLength));
.windowUpdate(eq(5), eq((long) 2 * messageFrameLength + paddingLength));
verify(frameWriter, timeout(TIME_OUT_MS))
.windowUpdate(eq(0), eq((long) 2 * messageFrameLength));

Expand Down