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: Eliminate buffer release when there is an error as caller still owns the buffer. #10537

Merged
merged 6 commits into from
Sep 15, 2023
Merged

netty: Eliminate buffer release when there is an error as caller still owns the buffer. #10537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80df75d
netty: Eliminate buffer release when there is an error as caller stil…
larry-safran Sep 5, 2023
426f1e6
Move resetting the composite reader index to before the release as it…
larry-safran Sep 6, 2023
6d97e70
Update netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
larry-safran Sep 14, 2023
4696ad5
Move release in tests from finally to catch block.
larry-safran Sep 14, 2023
2d4f1a2
Merge remote-tracking branch 'refs/remotes/origin/is10255' into is10255
larry-safran Sep 14, 2023
9a46d9e
Move release back to finally block in tests, but skip it if the merge…
larry-safran Sep 14, 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
11 changes: 3 additions & 8 deletions netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,11 @@ static void mergeWithCompositeTail(
composite.addFlattenedComponents(true, newTail);
// New tail's ownership transferred to the composite buf.
newTail = null;
in.release();
in = null;
// Restore the reader. In case it fails we restore the reader after releasing/forgetting
// the input and the new tail so that finally block can handles them properly.
composite.readerIndex(prevReader);
// Input buffer was successfully merged with the tail.
// Must be the last line in the try because we release it only on success.
in.release();
} finally {
// Input buffer was merged with the tail.
if (in != null) {
in.release();
}
// If new tail's ownership isn't transferred to the composite buf.
// Release it to prevent a leak.
if (newTail != null) {
Expand Down
16 changes: 12 additions & 4 deletions netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,18 +533,22 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,

try {
NettyAdaptiveCumulator.mergeWithCompositeTail(alloc, compositeThrows, in);
in = null; // On success it would be released
fail("Cumulator didn't throw");
} catch (UnsupportedOperationException actualError) {
assertSame(expectedError, actualError);
// Input must be released unless its ownership has been to the composite cumulation.
assertEquals(0, in.refCnt());
// Because of error, ownership shouldn't have changed so should not have been released.
assertEquals(1, in.refCnt());
// Tail released
assertEquals(0, tail.refCnt());
// Composite cumulation is retained
assertEquals(1, compositeThrows.refCnt());
// Composite cumulation loses the tail
assertEquals(0, compositeThrows.numComponents());
} finally {
if (in != null) {
in.release();
}
compositeThrows.release();
}
}
Expand All @@ -569,18 +573,22 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,

try {
NettyAdaptiveCumulator.mergeWithCompositeTail(mockAlloc, compositeRo, in);
in = null; // On success it would be released
fail("Cumulator didn't throw");
} catch (UnsupportedOperationException actualError) {
assertSame(expectedError, actualError);
// Input must be released unless its ownership has been to the composite cumulation.
assertEquals(0, in.refCnt());
// Because of error, ownership shouldn't have changed so should not have been released.
assertEquals(1, in.refCnt());
// New buffer released
assertEquals(0, newTail.refCnt());
// Composite cumulation is retained
assertEquals(1, compositeRo.refCnt());
// Composite cumulation loses the tail
assertEquals(0, compositeRo.numComponents());
} finally {
if (in != null) {
in.release();
}
compositeRo.release();
}
}
Expand Down